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

[Suggestion] Enhance the Heading component to support new text-wrap values #522

Open
sergioalvz opened this issue Jan 16, 2024 · 3 comments
Labels

Comments

@sergioalvz
Copy link

Hello everyone!

A few weeks ago, @simurai shared some insights regarding the text-wrap CSS property and its potential to simplify the adoption of Contentful for specific marketing pages. Currently, we manually break certain heading texts using <br> tags, which poses challenges for migration to Contentful and may introduce localization issues.

The good news is that text-wrap now offers native functionality to address this. You can read more about it here: https://developer.chrome.com/blog/css-wrapped-2023#textwrap.

Would it be possible to consider integrating this property with the Heading component? My suggestion is to introduce a new prop, similar to others like size or weight, which would allow us to pass specific text-wrap values to the component.

Let me know what you think!

@rezrah
Copy link
Collaborator

rezrah commented Jan 16, 2024

@sergioalvz - thanks for filing this request. We discussed it at Primer Brand office hours today.

In principle, we'd be happy to add this feature via a dedicated prop. The current browser support for text-wrap however, hasn't reached sufficient levels of support for us to adopt it in Primer components. This is particularly the case for text-wrap as it could pose accessibility issues in certain browsers, due to its unpredictability.

We'll review browser support over the next few quarters, and add this feature when support meets our GitHub policies (GitHub staff only).

@rezrah rezrah added the brand label Jan 16, 2024
@simurai
Copy link
Contributor

simurai commented Jan 17, 2024

We'll review browser support over the next few quarters, and add this feature when support meets our GitHub policies (GitHub staff only).

The last 3 Safari major versions seems quite strict? I think major versions of Safari only get released once a year? Which would mean once Safari adds the new text-wrap features, we'll still have to wait up to 3 more years? 🤔

Also, I see this more as a "progressive enhancement" instead an "all or nothing". Specifically for Contentful where responsively breaking titles to new lines is probably not easily done manually by a copy editor. So having it done by the browsers that already support it, can't hurt.

But before adding it directly to Primer Brand, we could also test it first a bit in specific places to see if the new text-wrap features cause any unforeseen problems.

@rezrah
Copy link
Collaborator

rezrah commented Jan 22, 2024

The last 3 Safari major versions seems quite strict? I think major versions of Safari only get released once a year? Which would mean once Safari adds the new text-wrap features, we'll still have to wait up to 3 more years? 🤔

I agree, but this is also our browser support commitment to users. AFAIK, this applies to all Primer libraries.

Also, I see this more as a "progressive enhancement" instead an "all or nothing".

We explored that avenue too. Current thinking is that it could pose accessibility problems due to lack of cross-browser support, and -for example - could give the false impression that fg/bg contrast ration is fine on some browsers, while it's a violation in others. We're primarily concerned about enabling support for a feature that isn't consistently applied just yet.

But before adding it directly to Primer Brand, we could also test it first a bit in specific places to see if the new text-wrap features cause any unforeseen problems.

👍 Sounds reasonable. I think it's a great feature, so we'd be keen to follow along with your experimentation. In the meantime, I'll check in with some other folks at GitHub to see where we draw the line on these new features and their adoption 🤞

cc. @danielguillan

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

No branches or pull requests

3 participants