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: remove select item flex to enable truncation, add left spacing for items #6410

Merged
merged 2 commits into from
Mar 3, 2024
Merged

Conversation

mojitane
Copy link
Contributor

@mojitane mojitane commented Mar 3, 2024

Removes unneeded flex on select items which prevented truncation of long titles from working. Adds left padding to better visualize hierarchy of groups and clickable items inside.

Description

Fixes this issue: #6400 (comment)
Currently, truncation in selects is not working because of flex set to the inner span of each select item. This removes this to fix the truncation issue and adds left padding to each item to further highlight the hierarchy.

On touch devices that don't support hover states it was hard to recognize groups and group items. This gives each item a 'pl-4` left padding to highlight these hierachies for all devies.

Before:
image

After:
image

This fix seems to be working fine. The downside of the left padding for items is, that this padding also exists when there aren't any groups used. This is the case on the Blog page.
Question is if this is relevant and improvement is needed to only add padding when groups are used.
Second question is if the previous set flex styles were intended to be used for anything down the road or are used on any page I am not aware of.

Example of Blog page item padding without any groups used:
image

Validation

Tested the Learn page referenced in the Issue. Also tested About page select and Blog page select.

Related Issues

Fixes #6400

Check List

  • I have read the Contributing Guidelines and made commit messages that follow the guideline.
  • I have run npx turbo lint to ensure the code follows the style guide. And run npx turbo lint:fix to fix the style errors if necessary.
  • I have run npx turbo format to ensure the code follows the style guide.
  • I have run npx turbo test to check if all tests are passing.
  • I've covered new added functionality with unit tests if necessary.

Removes unneeded flex on select items which prevented truncation of long titles from working.
Adds left padding to better visualize hierarchy of groups and clickable items inside.
@mojitane mojitane requested a review from a team as a code owner March 3, 2024 00:46
Copy link

vercel bot commented Mar 3, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Mar 3, 2024 1:17am

@ovflowd
Copy link
Member

ovflowd commented Mar 3, 2024

This fix seems to be working fine. The downside of the left padding for items is, that this padding also exists when there aren't any groups used. This is the case on the Blog page.

I guess you can use a CSS not selector or something from tailwind so that if the "section" element doesn't exist, no padding needed? But I guess if you just decrease the padding to be "pl-3" that might be enough?

@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Mar 3, 2024
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Mar 3, 2024
Copy link

github-actions bot commented Mar 3, 2024

Lighthouse Results

URL Performance Accessibility Best Practices SEO Report
/en 🟢 100 🟢 96 🟢 100 🟢 91 🔗
/en/about 🟢 100 🟢 96 🟢 100 🟢 92 🔗
/en/about/previous-releases 🟢 99 🟢 95 🟢 100 🟢 92 🔗
/en/download 🟢 100 🟢 95 🟢 100 🟢 91 🔗
/en/blog 🟢 100 🟢 92 🟢 96 🟢 92 🔗

Copy link

github-actions bot commented Mar 3, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 85%
80.18% (433/540) 79.65% (137/172) 73.07% (76/104)

Unit Test Report

Tests Skipped Failures Errors Time
88 0 💤 0 ❌ 0 🔥 4.364s ⏱️

Copy link
Member

@ovflowd ovflowd left a comment

Choose a reason for hiding this comment

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

LGTM! Awesome work here :)

@mojitane
Copy link
Contributor Author

mojitane commented Mar 3, 2024

This fix seems to be working fine. The downside of the left padding for items is, that this padding also exists when there aren't any groups used. This is the case on the Blog page.

I guess you can use a CSS not selector or something from tailwind so that if the "section" element doesn't exist, no padding needed? But I guess if you just decrease the padding to be "pl-3" that might be enough?

Testing this, I found the place where the flex styling I removed was used. On the download page when using together with an icon for system version.
After restoring the original flex style, I added a span wrapper to style the anonymous flex child. And then added the truncation and padding there. It is now pl-3, only applied via :has when also a group label exists and icons in selects work again.

Simple select
image

Select with icon
image

Select with group labels
image

Copy link
Contributor

@shanpriyan shanpriyan left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for your contribution :shipit:

Copy link
Contributor

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

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

LGTM ! Thanks for your first contribution

@ovflowd ovflowd added the fast-track Fast Tracking PRs label Mar 3, 2024
@ovflowd
Copy link
Member

ovflowd commented Mar 3, 2024

I'm fast tracking this, even tho this is a visual change, it's simple enough and if there are further objections we can always change.

@ovflowd ovflowd enabled auto-merge March 3, 2024 10:41
@ovflowd ovflowd added the github_actions:pull-request Trigger Pull Request Checks label Mar 3, 2024
@github-actions github-actions bot removed the github_actions:pull-request Trigger Pull Request Checks label Mar 3, 2024
@ovflowd ovflowd added this pull request to the merge queue Mar 3, 2024
Merged via the queue into nodejs:main with commit 004c16e Mar 3, 2024
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fast-track Fast Tracking PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Learn Page dropdown clickable entries not visibly distinct on mobile
4 participants