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: Adjust dropdown menu item layout for better text fitting #3159

Merged
merged 1 commit into from
Nov 4, 2024

Conversation

s1Sharp
Copy link
Contributor

@s1Sharp s1Sharp commented Oct 5, 2024

Summary

  • Updated the styling of .DownloadsDropdownMenu__MenuItem to improve text fitting and prevent text overlap.
  • Changed height to fit-content(25%) to ensure the menu items dynamically adjust to content size.
  • Added flex-grow: 1 to allow the items to expand and occupy available space within the menu.
  • Added padding-right: 20px to ensure text does not touch the right edge of the menu.

Device Information

This PR was tested on: Ubuntu 22.04

Screenshots

I have tested it in 3 different languages (English, Russian, French). Some of them had problems with text overlapping in the download menu tab. Other languages with long translations also have overlapping text.

Before Changes:
English Before Changes (English)
French Before Changes (French)
Russian Before Changes (Russian)
After Changes:
English After Changes (English)
French After Changes (French)
Russian After Changes (Russian)
Updated the styling of `Downloads Menu` to improve text fitting and prevent text overlap.

@mattermost-build
Copy link
Contributor

Hello @s1Sharp,

Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here.

@devinbinnie
Copy link
Member

Thanks for the PR @s1Sharp. The differences are definitely apparent with French/Russian, but with English it still looks like the text is too big for the menu size.

I'm wondering if we should increase the menu width a little bit, or better yet, force the inner div to be a bit smaller so that word wrap is turned on for that menu item that's stretched over. Thoughts?

@devinbinnie devinbinnie self-requested a review October 7, 2024 13:20
@devinbinnie devinbinnie added 2: Dev Review Requires review by a core committer 1: UX Review Requires review by a UX Designer labels Oct 7, 2024
@s1Sharp
Copy link
Contributor Author

s1Sharp commented Oct 7, 2024

I'm wondering if we should increase the menu width a little bit, or better yet, force the inner div to be a bit smaller so that word wrap is turned on for that menu item that's stretched over. Thoughts?

I assume you wanted to see something similar with line breaks in the English version?

Eng with right padding

This is easy to get. Need add padding from the right of 20px. There is already padding on the left.
padding-right: 20px;

@devinbinnie
Copy link
Member

I'm wondering if we should increase the menu width a little bit, or better yet, force the inner div to be a bit smaller so that word wrap is turned on for that menu item that's stretched over. Thoughts?

I assume you wanted to see something similar with line breaks in the English version?

Eng with right padding
This is easy to get. Need add padding from the right of 20px. There is already padding on the left. padding-right: 20px;

Yeah that looks great!

@s1Sharp
Copy link
Contributor Author

s1Sharp commented Oct 7, 2024

I'm wondering if we should increase the menu width a little bit, or better yet, force the inner div to be a bit smaller so that word wrap is turned on for that menu item that's stretched over. Thoughts?

I assume you wanted to see something similar with line breaks in the English version?
Eng with right padding
This is easy to get. Need add padding from the right of 20px. There is already padding on the left. padding-right: 20px;

Yeah that looks great!

For now it looks:

English After Changes (English)
French After Changes (French)
Russian After Changes (Russian)

Copy link
Member

@devinbinnie devinbinnie left a comment

Choose a reason for hiding this comment

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

Code LGTM! Thanks @s1Sharp!

Will need some input from our UX team.

@devinbinnie devinbinnie removed the 2: Dev Review Requires review by a core committer label Oct 8, 2024
@abhijit-singh abhijit-singh added the Build Apps for PR Builds signed builds for testing label Oct 10, 2024
Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

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

Thanks @s1Sharp! This is a great improvement! Preventing overflow and the 20px right padding helps so much.

I had just a couple of suggestions:

  1. Can we increase the width of the menu container based on the text, upto a maximum of 200px? Menu items don't have to be wrapped if we actually have the horizontal space.
  2. The menu items seem to be getting vertically squished a bit because of some options wrapping to multiple rows. Ideally, the minimum height for a menu item should not be less than 36px. We shouldn't have restrictions on the overall height of the popover menu and it can be taller.

Here's how it can look with these changes:

English:
image

French:
image

@mattermost-build
Copy link
Contributor

This PR has been automatically labelled "stale" because it hasn't had recent activity.
A core team member will check in on the status of the PR to help with questions.
Thank you for your contribution!

@devinbinnie
Copy link
Member

@s1Sharp Anything we can do to assist getting this finished?

@s1Sharp
Copy link
Contributor Author

s1Sharp commented Oct 28, 2024

@s1Sharp Anything we can do to assist getting this finished?

I tried to edit the current implementation so that the window for uploaded files would select the size dynamically. I couldn't find an elegant solution. But there is a working solution within the current implementation. I'll try to post it soon.

- Updated the styling of `.DownloadsDropdownMenu__MenuItem` to improve text fitting and prevent text overlap.
@s1Sharp
Copy link
Contributor Author

s1Sharp commented Oct 30, 2024

I tried to refactor the src/main/views/downloads Dropdown Menu View.ts and src/renderer/downloadsDropdownMenu.tsx files so that the DownloadsDropdownMenu internal shape resizing event sends its dimensions to DownloadsDropdownMenuView, but this will not work in the current implementation. Therefore, I increased the size of the embedded browser window in the src/renderer/constants.ts file. And also slightly changed the css styles for the drop-down menu according to a request from @abhijit-singh

For now it looks:

English After Changes (English)
French After Changes (French)
Russian After Changes (Russian)

Copy link

@abhijit-singh abhijit-singh 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 @s1Sharp! Looks good to me!

@devinbinnie devinbinnie removed the 1: UX Review Requires review by a UX Designer label Nov 4, 2024
@devinbinnie devinbinnie added the 4: Reviews Complete All reviewers have approved the pull request label Nov 4, 2024
@devinbinnie devinbinnie merged commit ebbee6d into mattermost:master Nov 4, 2024
19 checks passed
@amyblais amyblais added this to the v5.11.0 milestone Nov 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Build Apps for PR Builds signed builds for testing Contributor Hacktoberfest null Lifecycle/1:stale release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants