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

feat: implement content truncation for Action Plan cards (M2-7861) #541

Merged
merged 8 commits into from
Nov 20, 2024

Conversation

qiushihe
Copy link
Contributor

📝 Description

🔗 Jira Ticket M2-7861

This PR updates the Action Plan cards rendering code to implement a binary-searched based truncation algorithm. The algorithm will not only truncate at phrasal field level, but also at phrasal field content level, to ensure each card contains the maximum amount of content. Overflowing contents at the end of a card, and corresponding continuation content at the beginning of the following card, are connected by a set of ellipses.

For a walkthrough of implementation detail, please see this loom recording: https://www.loom.com/share/0f3fd7416f814e769ca1318e2ac13a28

📸 Screenshots

Text Truncation Start

Screenshot 2024-10-23 at 2 13 01 PM

Text Truncation Both Ends

Screenshot 2024-10-23 at 2 13 04 PM

Text Truncation End

Screenshot 2024-10-23 at 2 15 40 PM

**List Truncation Start/End **

Screenshot 2024-10-23 at 2 12 34 PM

List Truncation Both Ends

Screenshot 2024-10-23 at 2 13 15 PM

List Truncation when Rendered as Sentence

Screenshot 2024-10-23 at 2 14 34 PM

🪤 Peer Testing

Note: By default, the maximum page height is 2504px, which is very tall. So for testing, it's easier to run the code locally, then edit the usePageMaxHeight function of src/entities/activity/ui/items/ActionPlan/hooks.tsx, and make it return a smaller number (i.e. 512).

  1. Create an applet with an activity with a number of items
  2. Create a phrasal template item with abundance of content
  3. Go through the activity and reach the phrasal template step
  4. The Action Plan cards should be truncated

✏️ Notes

Although the AC of the ticket only specified truncation of text content, the implementation actually also truncate list items. The reason being: if a list is too long, it should just be truncated anyway. Otherwise, we'll have to deal with all sort of complication around list content overflowing/cut-off, etc. If we REALLY don't want to truncate list items, we can still set a flag in the code to turn that off.

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-541.d15zn9do8xbzga.amplifyapp.com

@qiushihe qiushihe force-pushed the feature/M2-7861-truncation branch from b729820 to 8359686 Compare October 23, 2024 21:29
@qiushihe qiushihe force-pushed the feature/M2-7861-truncation branch from 8359686 to 48c564b Compare October 23, 2024 21:32
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

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

Hey @qiushihe, amazing algorithmic work!! Your focus on this to take care of so many truncation edge cases is greatly appreciated. 🙏🏻❤️🙏🏻 I had some very minor comments but the code looks great otherwise.

I'm still encountering that same discrepancy in pagination between desktop and mobile browsers as I identified in your previous PR. With max page height set to 512, I'm getting quite a significant difference in # of pages generated:

Web (Chromium) Mobile (Safari)

@qiushihe
Copy link
Contributor Author

qiushihe commented Oct 25, 2024

@farmerpaul the issue with the mobile browser vs. desktop browser truncation height difference is due to font-size and line-height calculation. Those 2 values are not set directly, but are calculated using useXScaledDimension and useYScaledDimension respectively. I can try tweaking the formula a bit, but at the end of the day, mobile browser is essentially just a responsive view, with difference page width than the desktop browser (which can also have whatever page width people end up with). So it unlikely that the 2 can be made to be always completely identical.

I also addressed those other comments, so if you can take another look that would be great. I'll just be tweaking the font-size/line-height stuff in the background for a bit.

@qiushihe qiushihe requested a review from farmerpaul October 25, 2024 18:17
Copy link
Contributor

@farmerpaul farmerpaul left a comment

Choose a reason for hiding this comment

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

Hey @qiushihe, I recorded a Loom with a simple proposed solution to the aspect ratio issue, check it out: https://www.loom.com/share/898fea7503ef4f5696985e15eebf3fe4

See what you think, hopefully that will be adequate. I'll pre-approve regardless. 🙂

Edit: In my loom I demoed the solution with useXScaledDimension, but I supposed useYScaledDimension is actually the right one? Doesn't seem to make a whole lot of difference which one you use.

@qiushihe
Copy link
Contributor Author

That's actually really cool! I never thought about also scaling the max height value. And it seems easy enough, so I'll make the change. Thanks @farmerpaul!

@farmerpaul farmerpaul force-pushed the feature/M2-7861-truncation branch from 432635c to b81c010 Compare November 19, 2024 19:16
@farmerpaul farmerpaul self-requested a review November 19, 2024 19:16
@farmerpaul farmerpaul merged commit e132ce3 into dev Nov 20, 2024
3 checks passed
@farmerpaul farmerpaul deleted the feature/M2-7861-truncation branch November 20, 2024 21:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants