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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃挕 Proposal - Allow to edit which heading element is used in components #1879

Closed
YvesRijckaert opened this issue Feb 10, 2022 · 9 comments
Labels
needs review Proposal/bug that needs to be reviewed by maintainers proposal stale Used to mark when there was no activity for a set period of time

Comments

@YvesRijckaert
Copy link
Contributor

YvesRijckaert commented Feb 10, 2022

Forma 36 contribution proposal

The problem

When using the EntryCard component, I wanted to change the heading element from h1 to h3. But this does not seem possible right now.

The proposed solution

Being able to change the heading element with a prop.

@YvesRijckaert YvesRijckaert added proposal needs review Proposal/bug that needs to be reviewed by maintainers labels Feb 10, 2022
@YvesRijckaert
Copy link
Contributor Author

I can create a PR for this, just checking if it would be okay to do so.

@burakukula
Copy link
Contributor

Hey @YvesRijckaert! Sorry for the delay. I think it totally makes sense, I would even say that we have a bug right now, since it might actually create an incorrect structure. We should consider changing the default to something different than h1, but yeah, allowing the setting up of the tag here would be great and changing the default maybe. If you have some time to create a PR it would be great.

@denkristoffer
Copy link
Collaborator

Rendering the h1 shouldn't be a problem when it's inside article.

@YvesRijckaert
Copy link
Contributor Author

@denkristoffer I think we use this component in a lot of places, and often we would need more flexibility when it comes to which heading element to use.. I ran into this issue when using this component in compose.
But for example here in the web app an h2 or h3 would be more appropriate:
Screenshot 2022-02-14 at 14 31 48

@denkristoffer
Copy link
Collaborator

I am all for being able to change the element. I wanted to add information that article should create a new heading context which would mean using a h1 in one should be fine. However, this discussion made me double check, and it looks like this behaviour was never actually implemented in browsers even though the spec says it's fine 馃槺

@massao
Copy link
Contributor

massao commented Feb 15, 2022

I am all for being able to change the element. I wanted to add information that article should create a new heading context which would mean using a h1 in one should be fine. However, this discussion made me double check, and it looks like this behaviour was never actually implemented in browsers even though the spec says it's fine 馃槺

Didn't know that the browsers didn't implement this, good to know.
But IMHO even if it was implemented we should also give the option to change the heading, since we give the option to change the tag used as wrapper of the EntryCard and other components

@denkristoffer
Copy link
Collaborator

Coming back to this, I guess an API that fits with our existing guidelines would be something like passing an object to headingProps, which can then contain the as key?

<Card headingProps={{ as: 'h3' }} />

What do you think?

@github-actions
Copy link

github-actions bot commented Aug 9, 2022

Marking issue as stale since there was no acitivty for 30 days

@github-actions github-actions bot added the stale Used to mark when there was no activity for a set period of time label Aug 9, 2022
@Lelith
Copy link
Collaborator

Lelith commented Apr 15, 2024

this was fixed

@Lelith Lelith closed this as completed Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs review Proposal/bug that needs to be reviewed by maintainers proposal stale Used to mark when there was no activity for a set period of time
Projects
None yet
Development

No branches or pull requests

9 participants