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

Stop URl preview from covering message box #29215

Merged
merged 4 commits into from
Feb 11, 2025

Conversation

edent
Copy link
Contributor

@edent edent commented Feb 7, 2025

Fixes #23874 by adding a bit of padding.

1em should be sufficient to prevent the browser's URl preview from covering the entry box.

Before

Screenshot from 2025-02-07 09-16-55

After

Screenshot from 2025-02-07 09-16-40

Checklist

  • Tests written for new code (and old code if feasible).
  • New or updated public/exported symbols have accurate TSDoc documentation.
  • Linter and other CI checks pass.
  • I have licensed the changes to Element by completing the Contributor License Agreement (CLA)

Fixes element-hq#23874 by adding a bit of padding.

1em should be sufficient to prevent the browser's URl preview from covering the entry box.
@edent edent requested a review from a team as a code owner February 7, 2025 09:25
@edent edent requested review from t3chguy and florianduros February 7, 2025 09:25
@github-actions github-actions bot added the Z-Community-PR Issue is solved by a community member's PR label Feb 7, 2025
@t3chguy t3chguy requested a review from a team February 7, 2025 09:48
@ara4n
Copy link
Member

ara4n commented Feb 7, 2025

see also #29213

Copy link
Member

@florianduros florianduros left a comment

Choose a reason for hiding this comment

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

@edent Hi, thanks for the contribution!

We checked with our design team and we want to go instead with 8px. With char likes g, f, j etc we want more space to make it breathe more.

Also, we are using our design system tokens for spacing. (Old components like this one were written before we moved to a design system)

After that, the playwright tests need to be updated since the UI has changed.
Thank you and let me know if you need help.

@@ -35,6 +35,7 @@ Please see LICENSE files in the repository root for full details.
width: 100%;
flex: 0 0 auto;
margin-right: 2px;
padding-bottom: 1em;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
padding-bottom: 1em;
padding-bottom: var(--cpd-space-2x);

@edent
Copy link
Contributor Author

edent commented Feb 10, 2025

Are you sure that 8px is a suitable value? Not everyone's OS uses 8px as the height of characters - that's why I specifically chose an em unit.

Both Slack and Discord uses 24px for its padding between text entry and the bottom of the screen.

That said, this is your codebase, please proceed as you see fit.

@florianduros
Copy link
Member

florianduros commented Feb 10, 2025

I switch to a bigger font size and the composer component grows in a way as it doesn't overflow over the new padding:

image

With 1em:

image

I think we want to stick on var(--cpd-space-2x)

@t3chguy
Copy link
Member

t3chguy commented Feb 10, 2025

I switch to a bigger font size and the composer component grows in a way as it doesn't overflow over the new padding:

Right, but try increasing your browser font size, that'll make the url tooltip larger and overlap again.

@florianduros
Copy link
Member

@t3chguy Good point but if you change the font size in the user settings, the padding becomes enormous with em:

image

@edent
Copy link
Contributor Author

edent commented Feb 10, 2025

I just wanted to report the bug. I'll unsubscribe now. Feel free to fix it in whatever way makes sense for your codebase.

Copy link
Member

@florianduros florianduros left a comment

Choose a reason for hiding this comment

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

After internal discussion with the team we will stay with your solution of 1em. We will favorise the font size settings of the browser instead of element font size settings (accessibility matter).

@florianduros florianduros added this pull request to the merge queue Feb 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 11, 2025
@florianduros florianduros added this pull request to the merge queue Feb 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 11, 2025
@florianduros florianduros added this pull request to the merge queue Feb 11, 2025
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Feb 11, 2025
@florianduros florianduros added this pull request to the merge queue Feb 11, 2025
Merged via the queue into element-hq:develop with commit 8bf3ec2 Feb 11, 2025
28 checks passed
@edent edent deleted the patch-1 branch February 11, 2025 15:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-Enhancement Z-Community-PR Issue is solved by a community member's PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bottom URL status bar covers up text input field
4 participants