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

Adjust paragraph default font size to 16px #6007

Merged
merged 7 commits into from
Apr 11, 2024

Conversation

goplayoutside3
Copy link
Contributor

@goplayoutside3 goplayoutside3 commented Mar 29, 2024

Note that I'll do a design review with @seanmiller26 in-office next week to make any further font size adjustments.

Package

lib-grommet-theme
lib-react-components
app-content-pages
lib-classifier
I might be making further adjustments to app-project

Linked Issue and/or Talk Post

By default, most browsers use a font size value of 16px, not 14px. Rather than display paragraphs in a smaller than average font size, let's make our default paragraph 16px. I'll link any design resources from Sean here too.

❓ Should our font sizes in lib-grommet-theme actually be defined in rem?
Edit: Yes, I've opened #6028, but will leave the necessary changes for another PR.

❓ Should Grommet's Text component default 'medium' also be 16px?

Edit: No, we'll leave Text medium as 14px for now.

Describe your changes

  • Change Grommet's Paragraph component 'medium' size to 16px. This is also the default if no size prop is defined for a Paragraph throughout FEM.
  • Remove pxToRem() helper from lib-react-components. It's more efficient to simply use the rem unit directly in styling code than rely on Javascript for conversion.
  • Adjustments to subheadings on About Zooniverse pages.

How to Review

  • SpacedText and SpacedHeading stories should have the same styling on this branch and production.
  • New About Zooniverse have paragraphs and subheadings with font sizes closer to the intended Figma design.
  • Paragraphs on all FEM project pages are now 16px.
  • There's not a good way to test appearance of SubjectGroupCompasionTask because the only zoo project that uses it is Weird & Wonderful which is out of data.

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

  • Tests are passing locally and on Github
  • Documentation is up to date and changelog has been updated if appropriate
  • You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
  • FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
  • FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

  • All pages of a FEM project load: Home Page, Classify Page, and About Pages
  • Can submit a classification
  • Can sign-in and sign-out
  • The component is accessible

@goplayoutside3 goplayoutside3 added the design Design and/or UX label Mar 29, 2024
@coveralls
Copy link

coveralls commented Mar 29, 2024

Coverage Status

coverage: 81.922% (+0.03%) from 81.888%
when pulling 61e05cc on adjust-paragraph-font-size
into d9856fb on master.

@goplayoutside3
Copy link
Contributor Author

Noting here that we took a look at 16px paragraphs in-office and confirmed this PR is a good intermediate step. The above questions of converting Grommet font sizes to rem can be a future PR/discussion, and we'll likely leave the default Text component font size at 14px.

This PR can be approved once Sean recommends a line-height to match the new 16px default.

@seanmiller26
Copy link
Contributor

There isn't necessarily a universal standard for line-height values, however it is pretty common to use ~1.5rem. In Figma this looks pretty far spaced out, but I'm not sure how accurately it is displayed so i'll need to see a preview of this new value.

I'd like to see both 1.5rem and 1.4rem to do a quick comparison.

@seanmiller26
Copy link
Contributor

Just mirroring my comment in a Slack thread - the 1.4 spacing is a bit too tight. OK to move forward with the 1.5 line spacing.

@goplayoutside3
Copy link
Contributor Author

I updated a couple more places in the code where line-height was using units other than pixels, percentage, or unitless. There will have to be a larger sweep to address #6028, but this is a good start for now 👍

@mcbouslog
Copy link
Contributor

lib-user components look good! 🚀

Copy link
Contributor

@mcbouslog mcbouslog left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@github-actions github-actions bot added the approved This PR is approved for merging label Apr 11, 2024
@goplayoutside3 goplayoutside3 merged commit a0840cc into master Apr 11, 2024
7 checks passed
@goplayoutside3 goplayoutside3 deleted the adjust-paragraph-font-size branch April 11, 2024 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging design Design and/or UX
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants