[Banner] Rebuild with layout components #7365
Conversation
size-limit report 📦
|
kyledurand
left a comment
kyledurand
left a comment
There was a problem hiding this comment.
🔥 This looks awesome Aveline 👏
Just worried about removing the ID. I'm not sure what it was added for but we'd need some comms around removing it so it doesn't mess anything up in web. Maybe we should hold off on removing it until v11
| if (title) { | ||
| headingID = `${id}Heading`; | ||
| headingMarkup = ( | ||
| <div className={styles.Heading} id={headingID}> |
There was a problem hiding this comment.
Removing id is likely a breaking change. I imagine this was added for analytics or tracking of some kind. Makes me a bit uncomfortable to remove it but I want to
There was a problem hiding this comment.
Oh that's a good call about analytics tracking. The thinking in this case was the id was maybe used for semantics and aria labelling. I do have another PR to add id and could refactor this if we are game to merge that in #7363
There was a problem hiding this comment.
There is no way for teams to know the ID as it's generated with useUniqueId. I think this should be safe to remove. Other ID's though this could be the case.
There was a problem hiding this comment.
@BPScott do you remember why banners needed unique ids?
There was a problem hiding this comment.
It is because they don't use semantic heading elements so they get aria roles to link the ID. This should be fixed with HTML.
| contentID = `${id}Content`; | ||
| contentMarkup = ( | ||
| <div className={styles.Content} id={contentID}> | ||
| <Box paddingTop="05" paddingBottom="05"> |
There was a problem hiding this comment.
Wondering if we could do something like this until we can remove id:
| <Box paddingTop="05" paddingBottom="05"> | |
| <Box paddingTop="05" paddingBottom="05"> | |
| <div id={contentID} /> |
We'd have to ping the person who contributed the id prop to the component to see if this would be sufficient
### WHY are these changes introduced? Fixes #7341 <!-- link to issue if one exists -->
laurkim
force-pushed
the
layout-components-beta
branch
from
88856f7 to
e45e174
Compare
### WHY are these changes introduced? Fixes #7358 ### WHAT is this pull request doing? Refactors `Banner` component to use new layout primitive components
### WHY are these changes introduced? Fixes #7358 ### WHAT is this pull request doing? Refactors `Banner` component to use new layout primitive components
### WHY are these changes introduced? Fixes #7358 ### WHAT is this pull request doing? Refactors `Banner` component to use new layout primitive components
Fixes #7358 Refactors `Banner` component to use new layout primitive components
Fixes #7358 Refactors `Banner` component to use new layout primitive components
Fixes #7358 Refactors `Banner` component to use new layout primitive components
Fixes #7358 Refactors `Banner` component to use new layout primitive components
Fixes #7358 Refactors `Banner` component to use new layout primitive components
Fixes #7358 Refactors `Banner` component to use new layout primitive components
Fixes #7358 Refactors `Banner` component to use new layout primitive components
4 participants
WHY are these changes introduced?
Fixes #7358
WHAT is this pull request doing?
Refactors
Bannercomponent to use new layout primitive components