Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(Fix) Product description not correctly truncated #12635

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

wandji20
Copy link
Contributor

@wandji20 wandji20 commented Jul 1, 2024

What? Why?

Set CSS white-space property to normal and unify height and line-height CSS values

What should we test?

Test possible combinations of very long product descriptions with and without line breaks to confirm text is properly truncated.
Screenshot from 2024-07-01 19-16-14
image

Expected result

image

Release notes

N/A

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • Technical changes only

Dependencies

N/A

Documentation updates

N/A

@wandji20 wandji20 changed the title Wrap product summary description in div tag and truncate content in one line, ending with three dots for long text (Fix) Product description not correctly truncated #10685 Jul 1, 2024
@sigmundpetersen
Copy link
Contributor

Hi @wandji20 👋 Thank you
There are some spec failures, can you have look?

@wandji20 wandji20 force-pushed the wb-OFN-10685 branch 3 times, most recently from a9ff87c to 8d48a0b Compare July 2, 2024 09:28
@wandji20
Copy link
Contributor Author

wandji20 commented Jul 2, 2024

Hi @wandji20 👋 Thank you There are some spec failures, can you have look?

Hi @sigmundpetersen
All checks are passed after recommitting my work and force pushed.
However, I am on the fence as to why they failed with some Redis build error.

@sigmundpetersen
Copy link
Contributor

Thanks, this is now ready for review. One of our developers will come back to you if there is any feedback 👍

@dacook dacook self-requested a review July 2, 2024 23:49
Copy link
Member

@dacook dacook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello and welcome! Thanks for contribution, this looks like a good change but I think there might be an issue, can you please check?

app/webpacker/css/darkswarm/_shop-product-rows.scss Outdated Show resolved Hide resolved
@wandji20 wandji20 force-pushed the wb-OFN-10685 branch 3 times, most recently from 1a07bd4 to c1cd1de Compare July 3, 2024 17:55
@wandji20 wandji20 changed the title (Fix) Product description not correctly truncated #10685 (Fix) Product description not correctly truncated Jul 3, 2024
@sigmundpetersen
Copy link
Contributor

sigmundpetersen commented Jul 3, 2024

Hi @wandji20 the main branch build is currently broken, so the build errors here after rebase are not linked to your PR 👍

@wandji20
Copy link
Contributor Author

wandji20 commented Jul 3, 2024

Hi @wandji20 the main branch build is currently broken, so the build errors here after rebase is not linked to your PR 👍

Thank you @sigmundpetersen
So I can rebase again when failing specs are resolved

@wandji20 wandji20 requested a review from dacook July 3, 2024 19:03
@mkllnk mkllnk added the user facing changes Thes pull requests affect the user experience label Jul 5, 2024
Copy link
Member

@mkllnk mkllnk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

That looks like a great improvement. The confusion around this is that the original spec was to display two lines of text and truncate on the second line but nobody could make it work properly. So we decided to simplify and display only the first line of text. It looks like you fixed it now.

@rioug
Copy link
Collaborator

rioug commented Jul 7, 2024

@wandji20 , the broken specs have been fixed in the main branch (#12644 ) . Could you rebase your PR ? Thanks ?

@wandji20
Copy link
Contributor Author

wandji20 commented Jul 8, 2024

@wandji20 , the broken specs have been fixed in the main branch (#12644 ) . Could you rebase your PR ? Thanks ?

Hi @rioug
This PR is now rebased

Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 ! thanks for your help.

@drummer83 drummer83 self-assigned this Jul 9, 2024
@drummer83 drummer83 added pr-staged-au staging.openfoodnetwork.org.au and removed pr-staged-au staging.openfoodnetwork.org.au labels Jul 9, 2024
@drummer83
Copy link
Contributor

Hi @wandji20,
Thank you for working on this fix! 💪

I have tested your PR on our staging server. Here is what I found.

Before your PR

I could reproduce the issue with incorrect truncation in width and height - even though I had to delete the HTML tags in the product description (see 'How to reproduce' above):
image

After your PR

It's looking good in Firefox. The text is truncated correctly:
image

I have also added an additional line break after the first sentence, which is displayed correctly as well:
image

I have also tested the Safari 17.3 browser on Sonoma using Browserstack:
image

Summary

It's all looking good and I couldn't spot any regressions. 👍
This one is ready to go! Merging! 🚀 🥳

Thanks again! 🙏

@drummer83 drummer83 merged commit f0fd3bb into openfoodfoundation:master Jul 9, 2024
53 of 54 checks passed
@drummer83 drummer83 removed the pr-staged-au staging.openfoodnetwork.org.au label Jul 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
user facing changes Thes pull requests affect the user experience
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Product description not correctly truncated
6 participants