-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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 remaining characters UI being cut off with multiple langs selected on small devices #6252
base: main
Are you sure you want to change the base?
Conversation
a.flex, | ||
a.align_center, | ||
a.justify_center, | ||
a.gap_sm, | ||
styles.container, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi! New here so I'm not familiar with the overall styling but imo since you're bothering to make a styles object, move the flex styles into that too just to declutter the HTML a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured we could just reuse globally available styles as much as possible. I am also not sure what is the best approach here to comply with Bluesky guidelines. I will wait until someone from a core team jumps in and makes recommendations/suggestions here.
const shouldShowText = remainingCount <= 20 | ||
const shouldShowCircle = remainingCount > -10 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where are you getting 20 and -10 from? Are these experimentally computed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, is there ever a possibility that both shouldShowText
and shouldShowCircle
will be true? Will the UI elements conflict in that case? Would it be more sensible to use a single var?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, shouldShowText
and shouldShowCircle
can be true at the same time when remainingCount > -10 && remainingCount <= 20
, and that's when we display the number at the center of the progress circle.
Creating two variables was intentional, and I can not think of a way to use a single variable.
20 and -10 are from X
Let's get this rebased. |
I am not sure what's changed recently, but I cannot commit the merged changes because of the above issue. Husky fails on eslint for some reason. |
@gaearon I merged main by bypassing husky pre-commit hook with I think it's probably related to the recent eslint config update made in this commit. And maybe, husky doesn't handle it well with |
Fixes: #6178
Summary
The footer component of the composer dialog gets messed up when multiple langs are selected on small devices like iPhone SE 3rd Gen. We display the circle/pie progress bar next to the number of remaining characters, and it seems it can be optimized so that text lives inside the progress bar. It will create a room for the composer footer so that all components in it can fit nicely.
Solution
I referenced how X handles it, and it seemed quite reasonable.
https://github.com/user-attachments/assets/a29201db-3eec-472d-9f7d-fed1fb965f43
I'd love to receive any feedback/comments on these changes.