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.

Sign up for GitHub

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

Jump to bottom

Font improvements, loading fonts in Storybook, and adding more font Chromatic stories #4238

Merged
merged 22 commits into from
Apr 5, 2023
Merged

Font improvements, loading fonts in Storybook, and adding more font Chromatic stories #4238

merged 22 commits into from
Apr 5, 2023

Conversation

ktabors
Copy link
Member

@ktabors ktabors commented Mar 21, 2023 •
edited
Loading

Closes #4165 #4125 #3834

Adding a story to test #3983 which failed. Will be handled in another sprint.

  • Switched preloading of fonts for Chromatic and loading of fonts for Storybook to one typekit web project.
  • Updated the font family definitions to be more "correct" and prevented CCJK fonts from being italics.
  • Added chromatic stories to cover reported instances of components with italics that weren't correct with CCJK fonts.
  • Adding CCJK fonts for docs, for someone using the docs from another local and the translated strings use the correct fonts.
  • Pulled ActionButton, TextArea, and Textfield font stories into new languages package.

Note: Chrome dev tools sometimes show the font name and sometimes an id. The Firefox dev tools have a fonts tab that shows both the id and the font name.

✅ 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:

Run chromatic and confirm that the CCJK fonts are not italic.
Run chromatic locally and check via dev tools that the correct fonts are used. Look at the new "Languages" grouping for font stories.
Check the new provider locale zh-Hant font family Storybook story to see the rendered font via dev tools.

🧢 Your Project:

RSP

@ktabors ktabors changed the title WIP additional font loading for storybook and chromatic font stories WIP Loading more fonts in storybook and adding font chromatic stories Mar 21, 2023
ktabors
ktabors commented Mar 22, 2023
View reviewed changes
packages/@react-spectrum/textfield/chromatic/Textfield.chromatic.js
@@ -118,6 +118,27 @@ storiesOf('Textfield', module)
)}, false)
);

storiesOf('Languages/Textfield', module)
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding this story to confirm the state of #3983 with the correct fonts, which is currently a failure.

...ages/@react-spectrum/illustratedmessage/chromatic/IllustratedMessage.Languages.chromatic.tsx
type IllustratedMessageStory = ComponentStoryObj<typeof IllustratedMessage>;

export default {
title: 'Languages/IllustratedMessage',
Copy link
Member Author

@ktabors ktabors Mar 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Proposing all Chromatic font stories are in a "Languages" package, so we can more easily adjust locales without impacting other stories and they aren't spread all over the place.

@ktabors ktabors marked this pull request as draft March 22, 2023 18:17
@rspbot
Copy link

rspbot commented Mar 23, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 23, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 23, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 23, 2023

Build successful! 🎉

@ktabors ktabors changed the title WIP Loading more fonts in storybook and adding font chromatic stories Preloading in Storybook and adding more font Chromatic stories Mar 23, 2023
ktabors
ktabors commented Mar 23, 2023
View reviewed changes
packages/dev/docs/src/docs.css
@@ -51,6 +51,64 @@
font-weight: 900;
}

/** pulled from web project id: 'uei1lip' */
Copy link
Member Author

Choose a reason for hiding this comment

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

Allow docs to load the correct font if someone loads the page from an Arabic, Hebrew, Japanese, Korean, or Chinese locale and a component shows a i18n string.

Copy link
Member

Choose a reason for hiding this comment

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

i think we could do what we do here in our storybook

@ktabors ktabors marked this pull request as ready for review March 23, 2023 23:17
@devongovett
Copy link
Member

Note: let's test off the PR for this one because merging it would affect the docs in production.

@ktabors ktabors closed this Mar 23, 2023
@ktabors ktabors reopened this Mar 23, 2023
@rspbot
Copy link

rspbot commented Mar 23, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 27, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 27, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 28, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 28, 2023

Build successful! 🎉

devongovett
devongovett reviewed Mar 29, 2023
View reviewed changes
packages/dev/docs/src/docs.css Outdated
}
@font-face {
font-family: "myriad-arabic";
src: url("https://use.typekit.net/af/d1936b/00000000000000007735a2fd/30/l?unicode=AAAHygAAAAdhg4V2jXrgvTqablOmIuR90xJ6f7oYX7HHszHOhbuHChMcgQn5RM8D_2_09h-EDof5QbhEgbbA63nwPrW_c-fWpSq9I3W2wnPr2mXL5hwU9XCfpfCkLYjBT4lM8H7L4ONWX3ugfaTSwXm4HDcbyteaHuCpkuHywfcZB3Qmfrf-lhmtbmdiEYP1_3wmtwTqUe-84RpPif-WvZba-nEoqF8x54v53DpQrOTj48ldu33mIr3t5_p7J-7EIbKWiAPY-6fOlmzgfKcT52EWvLAE-pP79aefMQlHioFliM5EGken-uDgxR0sm5rfhBd4WUTkH-k-gPs7g-WyugAAAb0&features=NONE&v=3");
Copy link
Member

Choose a reason for hiding this comment

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

Do these ones need to have features=ALL as well?

@rspbot
Copy link

rspbot commented Mar 29, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 29, 2023

Build successful! 🎉

snowystinger
snowystinger reviewed Mar 29, 2023
View reviewed changes
packages/dev/docs/src/docs.css
@@ -51,6 +51,64 @@
font-weight: 900;
}

/** pulled from web project id: 'uei1lip' */
Copy link
Member

Choose a reason for hiding this comment

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

i think we could do what we do here in our storybook

snowystinger
snowystinger previously approved these changes Mar 29, 2023
View reviewed changes
Copy link
Member

@snowystinger snowystinger left a comment

Choose a reason for hiding this comment

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

I'm ok with my comments being followup if others are

@ktabors ktabors changed the title Preloading in Storybook and adding more font Chromatic stories Font improvements, preloading fonts in Storybook, and adding more font Chromatic stories Mar 29, 2023
@ktabors ktabors changed the title Font improvements, preloading fonts in Storybook, and adding more font Chromatic stories Font improvements, loading fonts in Storybook, and adding more font Chromatic stories Mar 29, 2023
@rspbot
Copy link

rspbot commented Mar 29, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Mar 29, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Apr 5, 2023

Build successful! 🎉

@rspbot
Copy link

rspbot commented Apr 5, 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' }

@devongovett devongovett merged commit f239d0b into main Apr 5, 2023
@devongovett devongovett deleted the ccjk_font branch April 5, 2023 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LFDanLu LFDanLu LFDanLu left review comments

@devongovett devongovett devongovett approved these changes

@snowystinger snowystinger snowystinger approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

IllustratedMessage uses Italics font-style for CCJK
5 participants
@ktabors @rspbot @devongovett @snowystinger @LFDanLu