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 dropdown cutoff issue for long text in mobile view (#7035) #7039

Closed
wants to merge 3 commits into from

Conversation

aqibzaib
Copy link

@aqibzaib aqibzaib commented Sep 13, 2024

Description

Fixes #7035

This pull request resolves the issue where the dropdown menu was being cut off when displaying long text on mobile devices. The changes implemented include:

  • Expanding the dropdown width to utilize the full available space on mobile devices.
  • Updated: Removing horizontal scrolling and allowing the text to wrap to a new line when it exceeds the width of the dropdown.

Before and After Videos

Validation

To verify that the issue has been resolved:

  1. ✔️ Ensure that the dropdown now supports text wrapping for long text and does not get cut off.
  2. ✔️ Confirm that the dropdown width is expanded appropriately on mobile devices.
  3. ✔️ Review the provided before and after videos to validate the visual improvements.

Checklist

  • ✔️ I have read the Contributing Guidelines and ensured that my commit messages follow the guidelines.
  • ✔️ I have run npm run format to ensure the code follows the style guide.
  • ✔️ I have run npm run test to confirm that all tests pass.
  • ✔️ I have run npx turbo build to verify that the website builds without errors.

@aqibzaib aqibzaib requested a review from a team as a code owner September 13, 2024 10:06
Copy link

vercel bot commented Sep 13, 2024

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview Sep 18, 2024 4:18am

@AugustinMauroy
Copy link
Contributor

not fan of the new behavior. I thinks making a breakline when text is too long should be better than scrool

@aqibzaib
Copy link
Author

not fan of the new behavior. I thinks making a breakline when text is too long should be better than scrool
Thanks for your valuable feedback! 😊 I've updated the select options to wrap, so no more endless scrolling adventures. Easy reading from now on!

image

@TenzDelek
Copy link
Contributor

TenzDelek commented Sep 15, 2024

Why is the Vercel deployment failing (server error)on other page except homeimage

@ovflowd
Copy link
Member

ovflowd commented Sep 15, 2024

Why is the Vercel deployment failing (server error)on other page except homeimage

That is incompatible because you made changes on a server-side component by adding client-side specific APIs. (useRef, useEffect)

I'd suggest on finding alternatives without using client-side-specific APIs.

@@ -66,13 +68,32 @@ const Select: FC<SelectProps> = ({
return mappedValues;
}, [values]);

useEffect(() => {
Copy link
Member

Choose a reason for hiding this comment

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

This sounds like a very inefficient way of solving an issue that could be solved only with CSS.

@TenzDelek
Copy link
Contributor

Haha , I thought it was some error from my device network
@aqibzaib you can make changes as per suggested by @ovflowd .
I suggest trying with ellipse property tailwind

Comment on lines +136 to +138
.textWrap {
text-wrap: wrap !important ;
}
Copy link
Member

Choose a reason for hiding this comment

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

Thank you for your contribution 🙇

As Claudio stated, I also think we should solve these problems in CSS

We use Tailwind classes to style CSS files;
https://github.com/nodejs/nodejs.org/blob/main/COLLABORATOR_GUIDE.md#example-of-a-css-module

In addition, we are careful not to define important properties and classes unless necessary.

You can make an example arrangement like this;

.inline {
  ... some code

  .text span > span {
    @apply max-w-fit
      text-wrap;
  }
}

@mikeesto
Copy link
Member

Thanks for your efforts here. I'm going to close this in favour of #7046 which has now landed.

@mikeesto mikeesto closed this Sep 20, 2024
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.

Cut off dropdown for long form text
6 participants