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 diacritics in textfields #4579

Merged
merged 10 commits into from
Jun 3, 2023
Merged

Fix diacritics in textfields #4579

merged 10 commits into from
Jun 3, 2023

Conversation

snowystinger
Copy link
Member

@snowystinger snowystinger commented May 24, 2023

Closes #3983

✅ Pull Request Checklist:

  • Included link to corresponding React Spectrum GitHub Issue.
  • Added/updated unit tests and storybook for this change (for new code or code which already has tests).
  • Filled out test instructions.
  • Updated documentation (if it already exists for this component).
  • Looked at the Accessibility Practices for this feature - Aria Practices

📝 Test Instructions:

Go to new story, change the locale, a different string will appear for each one. The big difference is if you toggle Zalgo text, here you can see the full range of area in which we can display a diacritic has been increased. It's gibberish, just shows where the cut off is. If you pull this locally, you can ever the change and compare to where it used to be cut off.

Note, this is safe, we'll never extend out beyond the bounds of the textfield itself, the text is overflow hidden if it tries to. With the exception of Zalgo and maybe some Tibet strings which Spectrum doesn't support anyways, the diacritics should never extend this far in the vertical range.

🧢 Your Project:

@rspbot
Copy link

rspbot commented May 24, 2023

Copy link
Member

@yihuiliao yihuiliao left a comment

Choose a reason for hiding this comment

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

checked in firefox, safari, and chrome on mac and it looks good! doesn't seem to get cut off

reidbarber
reidbarber previously approved these changes May 25, 2023
Copy link
Member

@reidbarber reidbarber left a comment

Choose a reason for hiding this comment

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

LGTM, we should run chromatic

@snowystinger
Copy link
Member Author

LGTM, we should run chromatic

yeah, i'm running it on main right now, then I'll run it here
I didn't add a chromatic story for this because @ktabors is adding locale specific strings to chromatic in another PR

@rspbot
Copy link

rspbot commented May 25, 2023

@rspbot
Copy link

rspbot commented May 25, 2023

@rspbot
Copy link

rspbot commented May 30, 2023

@rspbot
Copy link

rspbot commented Jun 1, 2023

@rspbot
Copy link

rspbot commented Jun 2, 2023

@rspbot
Copy link

rspbot commented Jun 2, 2023

@rspbot
Copy link

rspbot commented Jun 2, 2023

@rspbot
Copy link

rspbot commented Jun 2, 2023

@rspbot
Copy link

rspbot commented Jun 2, 2023

## API Changes

unknown top level export { type: 'identifier', name: 'Column' }
unknown top level export { type: 'identifier', name: 'Column' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }
unknown type { type: 'link' }

@snowystinger snowystinger merged commit f1345d8 into main Jun 3, 2023
@snowystinger snowystinger deleted the fix-textfield-diacritics branch June 3, 2023 00:00
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.

TextField issue with diacritics not fully visible
6 participants