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: layout word break #6712

Merged
merged 2 commits into from May 17, 2024
Merged

fix: layout word break #6712

merged 2 commits into from May 17, 2024

Conversation

araujogui
Copy link
Member

Description

Change layout's word break to normal

Validation

image

Related Issues

Fixes #6711

Check List

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

@araujogui araujogui requested a review from a team as a code owner May 5, 2024 16:50
Copy link

vercel bot commented May 5, 2024

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

Name Status Preview Updated (UTC)
nodejs-org ✅ Ready (Inspect) Visit Preview May 7, 2024 9:47am

Copy link

github-actions bot commented May 6, 2024

Lighthouse Results

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

Copy link

github-actions bot commented May 6, 2024

Unit Test Coverage Report

Lines Statements Branches Functions
Coverage: 91%
90.04% (588/653) 76.08% (175/230) 92.18% (118/128)

Unit Test Report

Tests Skipped Failures Errors Time
128 0 💤 0 ❌ 0 🔥 6.004s ⏱️

@canerakdas
Copy link
Member

It seems to overflow content for low-resolution mobile browsers, for example;

https://nodejs-jow7mncue-openjs.vercel.app/en/blog/release/v20.12.0

IMO it might be better to use xs:break-words for mobile resolution

@bmuenzenmeyer
Copy link
Collaborator

yeah - we have dueling solutions here now and we need to slow down and consider our use cases and options

Copy link
Member

@canerakdas canerakdas left a comment

Choose a reason for hiding this comment

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

LGTM

@NiedziolkaMichal
Copy link

You guys may consider text-align: justify:
image

@krave1986
Copy link

I would suggest try

word-break: normal;
overflow-wrap: anywhere;
hyphens: auto;

This will both keep the result closest to the current design and bring hyphens to broken words at the end of the lines.

Another benefit would be that we don't need to concern media query for such cases further.

@ovflowd
Copy link
Member

ovflowd commented May 8, 2024

Look we also already tried the hyphens approach. Each approach has its own cons and pros and different side effects.

I think is worth going over git history to see what was already attempted and what not and if whatever proposed works well on all scenarios

@tniessen
Copy link
Member

I came across this PR after running into this odd issue and trying to fix it myself. For me, personally, justified alignment and normal word break work fine in combination.

@ovflowd
Copy link
Member

ovflowd commented May 10, 2024

@araujogui could you go over the considerations that both me, Caner and Brian mentioned? 👀

@araujogui
Copy link
Member Author

Found out that word-break was introduced in #6680, so I think we can assume that until then it was the browsers defaults (normal). The issue solved in #6680 didn't come back with this PR.

@ovflowd ovflowd added this pull request to the merge queue May 17, 2024
Merged via the queue into nodejs:main with commit 242c4d5 May 17, 2024
15 checks passed
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.

Blog Word Break