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

Heading component: improve props to separate level from size #52281

Open
afercia opened this issue Jul 4, 2023 · 0 comments
Open

Heading component: improve props to separate level from size #52281

afercia opened this issue Jul 4, 2023 · 0 comments
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended

Comments

@afercia
Copy link
Contributor

afercia commented Jul 4, 2023

Description

Splitting this out from #52271 (comment) where the actual use case is the ability to set a correct heading level and render a heading font size that is different from the default one.

Please see this comment: #52271 (comment)

I was pretty confused by the way the Heading component handles the level prop. To me, 'level' only means the actual heading level: whether it's a h1, h2, h3, etc,

Instead, the level prop can also determine the heading font size.

The way the code works right now is a bit confusing and misleading. Developers should be educated that the heading elvel must be set based on the headings hierarchy on a page. The actual heading font size is a separate concept and only matters for visual presentation.

In fact, it is perfectly valid to use, for example, a H2 heading level and visually present it with a small font size.

A way to do this given the current code would be:

<Heading as="h2" level={ 6 }>

This makes still sense because the level prop actually uses a value that is included within the 1-6 range.

However, it is possible to pass a value that is not included in the 1-6 range, for example:

<Heading as="h2" level={ 48 }>

will actually set the heading font size to 48 pixels.

To me, the prop name level hints to the actual 1-6 heading levels. It should not be used to set arbitrary font sizes. That's pretty confusing. More importantly, it doesn't contribute to educate developers that level and size are two separate concepts.

Worth also noting that the logic in getHeadingFontSize actually checks whether the passed value is included or not within the 1-6 heading levels, implicitly allowing for values that are other than 1-6.

Step-by-step reproduction instructions

Noting that the storybook doesn't allow to set level values other than 1-6, while it is actually possible to do so in the code.

  • Alter this line of code and make it <Heading as="h2" level={ 48 }>
  • Go to Site Editor > Templates > Manage all templates
  • Observe the template headings are H2 with a font size of 48 pixels
  • Observe there's no errors or warnings in the console.
  • Run the tests: npm test
  • Observe there's no error or warnings.

Screenshots, screen recording, code snippet

No response

Environment info

No response

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@afercia afercia added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Jul 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

No branches or pull requests

1 participant