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: prevent text overflow in Japanese #548

Merged
merged 1 commit into from
Dec 4, 2024
Merged

Conversation

melzreal
Copy link
Contributor

@melzreal melzreal commented Nov 29, 2024

Description

In Japanese, having the single character broken into another line, provides another word meaning. We want to make sure the text is only broken at points that still preserve the message meaning of "Last activity"

Screenshots

Before

image

After

image

Checklist

  • 📗 all commit messages follow the conventional commits standard
  • ⬅️ changes are compatible with RTL direction
  • ♿ Changes to the UI are tested for accessibility and compliant with WCAG 2.1.
  • 📝 changes are tested in Chrome, Firefox, Safari and Edge
  • 📱 changes are responsive and tested in mobile
  • 👍 PR is approved by @zendesk/vikings

@melzreal melzreal requested a review from a team as a code owner November 29, 2024 16:17
@melzreal melzreal force-pushed the melreal/adjust-table branch from 70eace4 to 48ae36e Compare November 29, 2024 16:21
Copy link
Contributor

@luis-almeida luis-almeida left a comment

Choose a reason for hiding this comment

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

The style.css is the compiled "dist" file. These changes should go in the sass partial.

When it comes to the fix:

  • Have we gotten any feedback from localization that text-wrap: nowrap; is ok with other locales as well? I don't see why it wouldn't be but maybe worth checking
  • What happens when the text is larger than the container? Will it stretch and break the layout?

Copy link
Contributor

@Fredx87 Fredx87 left a comment

Choose a reason for hiding this comment

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

We shouldn't edit the style.css directly, as mentioned in the readme: https://github.com/zendesk/copenhagen_theme#stylesheet-and-javascript. In fact it is automatically hidden as a generated file.

We need to edit the source .scss file instead:

.

Do we also need to change the padding of the dd element?

@melzreal melzreal force-pushed the melreal/adjust-table branch from 48ae36e to 124eac8 Compare December 2, 2024 10:32
@melzreal
Copy link
Contributor Author

melzreal commented Dec 2, 2024

@luis-almeida

The style.css is the compiled "dist" file. These changes should go in the sass partial.

Thank you, have discarded the changes, amended the correct file and run yarn to re-generate the files

When it comes to the fix:

  • Have we gotten any feedback from localization that text-wrap: nowrap; is ok with other locales as well? I don't see why it wouldn't be but maybe worth checking
  • What happens when the text is larger than the container? Will it stretch and break the layout?

Good question, I had checked quite a few languages https://pandora-v2.zende.sk/resources/accounts/184478 - and just realised the overflow wrap breaks for Greek 🤦
but for it to look good in both languages only if the container gets bigger, because the text needs more space.. I'm going to have a go at it again

@melzreal melzreal force-pushed the melreal/adjust-table branch from 2d6af3a to 47e8ded Compare December 2, 2024 12:08
@luis-almeida
Copy link
Contributor

Would line-break: strict; address the issue?
Would be nice if we could keep the text-wrap not to have to change the design.

@melzreal melzreal force-pushed the melreal/adjust-table branch from 47e8ded to e3fa82b Compare December 2, 2024 13:22
@melzreal
Copy link
Contributor Author

melzreal commented Dec 2, 2024

Would line-break: strict; address the issue? Would be nice if we could keep the text-wrap not to have to change the design.

No, I did try but it had no effect. Have changed this to slighly reduce the container padding for the text by 5px while increasing the container 3%, so the text now looks good on most viewpoints - should I put this to Guntis for approval? Or would you recommend a different approach?

Do we also need to change the padding of the dd element?

@Fredx87 I ended up changing the container instead to allow a little more text space due to Greek looking not good with overflow wrap for the text

image image

@luis-almeida
Copy link
Contributor

Looks ok in the screenshots you added but with the full layout I think it looks a bit broken without much space between the containers:

Screenshot 2024-12-03 at 14 20 56

I'm not a designer so we can still try to run this approach by Guntis but since it's not consistent with the other sidebars it would be nice to find another solution.

Would line-break: strict; address the issue? Would be nice if we could keep the text-wrap not to have to change the design.

No, I did try but it had no effect.

Did you try it along with the text-wrap: nowrap? It's meant to be used without setting any text-wrap, and if so it will keep the text-wrapping but respect asian languages. From the documentation:

Break text using the most stringent line break rule.

When I use it by itself I do see a a difference in the rendering. Note the 3rd field no longer breaks at the same place:

Before
image

After
image

If we can confirm with a Japanese speaker that the second image reads correctly, then IMO that would be the correct fix.

@melzreal melzreal force-pushed the melreal/adjust-table branch from e3fa82b to 7c47eac Compare December 4, 2024 10:14
@melzreal
Copy link
Contributor Author

melzreal commented Dec 4, 2024

Did you try it along with the text-wrap: nowrap? It's meant to be used without setting any text-wrap, and if so it will keep the text-wrapping but respect asian languages.

I had expected the rule to not allow it to break at all even with nowrap disabled, hence finding it wasn't affecting the text - I didn't realise the difference

If we can confirm with a Japanese speaker that the second image reads correctly, then IMO that would be the correct fix.

Confirmed with Yuko (native Japanese speaker) that its indeed fine to break at that point

- In Japanese, allowing the text to wrap randomly
changes the meaning of the word. Have confirmed with a Japanese
speaker that using strict to allow the line to break
still retains the correct meaning.

- Used npm run build to make sure the asset minification isnt lost
@melzreal melzreal force-pushed the melreal/adjust-table branch from 7c47eac to 1c568c7 Compare December 4, 2024 11:59
@melzreal melzreal merged commit ee039ac into master Dec 4, 2024
5 checks passed
@melzreal melzreal deleted the melreal/adjust-table branch December 4, 2024 12:19
@zd-svc-github-copenhagen-theme
Copy link
Collaborator

🎉 This PR is included in version 4.2.5 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants