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

change the logic of show more from number of lines to number characters #9452

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

KaleemNeslit
Copy link
Collaborator

Link to Issue

Closes: #9276

Description of Changes

  • Change the "show more " from number of lines to length Or character .
  • currently the length of content is 500 character output is attached below for better understanding

image

@Israellund Israellund requested a review from mzparacha October 4, 2024 20:58
@KaleemNeslit
Copy link
Collaborator Author

Screen.Recording.2024-10-10.at.1.39.06.PM.mov

after resolving the conflict

@masvelio
Copy link
Contributor

@KaleemNeslit what's the status here? Please make sure that PRs do not getting stale. It's your responsibility to merge it as soon as possible 🙏

@masvelio
Copy link
Contributor

@KaleemNeslit

@KaleemNeslit
Copy link
Collaborator Author

@KaleemNeslit

@masvelio , this PR needs review. I have addressed the comments and requested reviews from 4 people, but 2 are still pending, even though it's been many days.

@KaleemNeslit
Copy link
Collaborator Author

@masvelio @Israellund @burtonator @mzparacha Whenever you have a chance, could you please review my PR? It's been in a stale state for a few days. Thanks!

@masvelio
Copy link
Contributor

@mzparacha could you take a look on this one?

@mzparacha
Copy link
Contributor

mzparacha commented Oct 25, 2024

Noticed some inconsistencies b/w the master and the current implementation.

Current:

Screen.Recording.2024-10-25.at.6.37.35.PM.mov

Master:

Screen.Recording.2024-10-25.at.6.39.04.PM.mov

Just for more context: this quill text cutoff has historically caused many inconsistencies b.w devices (iOS safari for instance #8611) and thread bodies (some of which you can see in the master branch video above). Lets aim to fix the multiline \n and markdown issues (plz compare the threads in videos for reference).

cc: @KaleemNeslit

@KaleemNeslit
Copy link
Collaborator Author

Noticed some inconsistencies b/w the master and the current implementation.

Current:

Screen.Recording.2024-10-25.at.6.37.35.PM.mov
Master:

Screen.Recording.2024-10-25.at.6.39.04.PM.mov
Just for more context: this quill text cutoff has historically caused many inconsistencies b.w devices (iOS safari for instance #8611) and thread bodies (some of which you can see in the master branch video above). Lets aim to fix the multiline \n and markdown issues (plz compare the threads in videos for reference).

cc: @KaleemNeslit
yes in master we are showing more base on number of line cutoff , if 6 so after 6 \n it will show "show more "
so we change the logic inti character length . if you have 100 character it will show "show more " it does not matter how many /n or line you have .
@mzparacha what should we do in that case ?

@mzparacha
Copy link
Contributor

Lets aim to fix the multiline \n and markdown issues (plz compare the threads in videos for reference).

@KaleemNeslit
Copy link
Collaborator Author

updates we need to update some logic related to the cutofflines and maxChars so i am converting PR as draft to implement the requirements

@KaleemNeslit KaleemNeslit marked this pull request as draft October 28, 2024 14:09
@KaleemNeslit KaleemNeslit marked this pull request as ready for review December 25, 2024 17:45
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.

Large text blocks not triggering "Show more"
6 participants