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

Add width utility to limit line length for readability #2410

Merged
merged 5 commits into from Jun 9, 2023

Conversation

dylanatsmith
Copy link
Contributor

@dylanatsmith dylanatsmith commented Mar 31, 2023

What are you trying to accomplish?

Add a utility class to limit line length to a more comfortable number of characters.

This screen recording shows the effect of toggling between max-width: 100% and max-width: 75ch on an example GitHub settings page.

Screen.Recording.2023-01-13.at.15.41.19.mov

What approach did you choose and why?

New width- utility class.

What should reviewers focus on?

Does 65 feel right? @ericwbailey notes in a comment below that line lengths of 80 characters or fewer would be WCAG compliant. In my experience, 70–75 feels a little better for paragraphs, but shorter can be nicer in product UI due to the shorter overall strings.

Can these changes ship as is?

  • Yes, this PR does not depend on additional changes. 🚢

Note: I’m not familiar enough with Primer processes to know what else we’d need to do to ship this, but it’s a standalone class addition.

@changeset-bot
Copy link

changeset-bot bot commented Mar 31, 2023

🦋 Changeset detected

Latest commit: e7babb4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/css Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Contributor

@ericwbailey ericwbailey left a comment

Choose a reason for hiding this comment

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

I think this would be a very welcome addition.

Part of WCAG Success Criterion 1.4.8 Visual Presentation is guaranteeing:

Width is no more than 80 characters or glyphs (40 if CJK).

SC 1.4.8 is also level AAA, so it technically does not apply to GitHub's minimum WCAG level of support, which is AA. That said, a comfortable maximum line width for English content and helps a lot with reading, comprehension, retention, Dyslexia, ESL, and other concerns.

I also appreciate the comment specifically calls out text content as what it should be used for.

@dylanatsmith dylanatsmith marked this pull request as ready for review April 3, 2023 11:33
@dylanatsmith dylanatsmith requested a review from a team as a code owner April 3, 2023 11:33
Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

Would this utility be added to a parent container (e.g. replace container-md with width-comfortable)? Or manually added on each <p>. For the later it might create this stair effect where things have different widths:

Text vs heading Text with different font-size
Screen Shot 2023-04-06 at 12 16 26 Screen Shot 2023-04-06 at 12 37 53

I guess it's a good thing for readability, but does that look visually weird?

Also if one settings page would start using this utility class, we probably should add it to all settings pages and use the same behavior? Maybe more a .width-comfortable p selector? Or make it an option on the Layout component.

@dylanatsmith
Copy link
Contributor Author

Would this utility be added to a parent container (e.g. replace container-md with width-comfortable)? Or manually added on each <p>. For the later it might create this stair effect where things have different widths:
Text vs heading Text with different font-size
Screen Shot 2023-04-06 at 12 16 26 Screen Shot 2023-04-06 at 12 37 53

I guess it's a good thing for readability, but does that look visually weird?

Also if one settings page would start using this utility class, we probably should add it to all settings pages and use the same behavior? Maybe more a .width-comfortable p selector? Or make it an option on the Layout component.

Sorry @simurai, suuuuper late coming back to this.

My thinking was that it could be used on "problem" elements, like individual p or label tags, to fix individual cases.

It could also be used on parent containers but the risk would be that there are elements we want to keep full-width and it causes problem. Imagine where we have a div with a banner and then a paragraph, for example.

I could run a few tests if that'd help illustrate intended usage better.

Copy link
Contributor

@simurai simurai left a comment

Choose a reason for hiding this comment

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

It could also be used on parent containers but the risk would be that there are elements we want to keep full-width and it causes problem.

On a parent container, I feel like it should only be used with caution, since child elements could have different font-sizes.

My thinking was that it could be used on "problem" elements, like individual p or label tags, to fix individual cases.

But, yeah.. when used intentionally for certain text elements, that should be fine. 👍

@dylanatsmith
Copy link
Contributor Author

Given we have approval here, are we able to merge? I’m not familiar with the process (and don’t think I have write permissions). @simurai

@simurai
Copy link
Contributor

simurai commented Jun 6, 2023

and don’t think I have write permissions

Ahh.. right. And I lost my write permission. 😅

Maybe @langermank still has that power?

@langermank langermank merged commit 344224f into primer:main Jun 9, 2023
10 of 12 checks passed
@primer-css primer-css mentioned this pull request Jun 9, 2023
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.

None yet

4 participants