feat(side sheet): side sheet component #532
Conversation
|
Demo will be published at https://apps.inindca.com/common-ui-docs/genesys-webcomponents/feature/COMUI-2931-Side_Sheet |
| <gux-side-sheet-heading-beta slot="heading" level="3"> | ||
| Side sheet title | ||
| </gux-side-sheet-heading-beta> |
There was a problem hiding this comment.
I dont think this component needs the beta tag since its just styling the heading ? Also I am sort of confused by this component. I cannot find anywhere in the designs where it specifies the use of heading tags.
Could you not just have a component that has a default slot and the user can slot whatever they like eg a heading tag and you style whatever element is passed with the same styling like,
::slotted(*) {
padding: 0;
margin: 0;
font-family: var(--gse-ui-dynamicSideSheet-heading-fontFamily);
font-size: var(--gse-ui-dynamicSideSheet-heading-fontSize);
font-weight: var(--gse-ui-dynamicSideSheet-heading-fontWeight);
line-height: var(--gse-ui-dynamicSideSheet-heading-lineHeight);
color: var(--gse-ui-dynamicSideSheet-headerColor);
}
so regardless of what the consumer slots it will always get the same styling so the api might look like,
<gux-side-sheet-heading slot="heading"><h1>This is a heading</h1> </gux-side-sheet-heading>
There may be an issue if a user was to slot just a text node as ::slotted(*) cannot target that but you could just create a light.scss file and do it there like,
gux-side-sheet-beta {
gux-side-sheet-heading {
... same styles as from the component
}
}
I may have missed something from the design so let me know.
There was a problem hiding this comment.
@gavin-everett-genesys
"I cannot find anywhere in the designs where it specifies the use of heading tags."
UX just specifies a heading but we should allow for different heading levels so people can create the correct document outline for accessibility, so we shouldn't hardcode a heading level like in the modal.
"default slot and the user can slot whatever they like eg a heading tag"
Yes, that would be ideal & I was originally slotting in h1, h2, h3...tags but there are global styles on the h* tags that are taking precedence. I'd like to avoid light dom styles here if at all possible.
"issue if a user was to slot just a text node "
There will have to be some tag present if even just to be able to apply the slot attribute so I don't think you can just insert a text node without some sort of wrapper.
Ping me if you have a better idea & we can tinker with it.
| <h1>Side Sheet</h1> | ||
| </div> | ||
|
|
||
| <gux-side-sheet-beta size="medium"> |
There was a problem hiding this comment.
I think you should include examples of small and large also.
There was a problem hiding this comment.
@gavin-everett-genesys yes, 100%, just looking for API feedback at the moment.
There was a problem hiding this comment.
API wise my biggest question is how much control we want of this component. At the moment it is mainly just a presentational component and most of the layout stuff is left up to the app using the component.
I think we will need to see an implementation of the modal version too before we nail down an api.
| <gux-side-sheet-beta size="medium"> | ||
| <gux-icon slot="icon" icon-name="fa/diamond-regular" size="medium"></gux-icon> | ||
| <gux-side-sheet-heading-beta slot="heading" level="3"> | ||
| Side sheet title |
There was a problem hiding this comment.
Should 'gux-side-sheet-heading-beta' have the icon slot not gux-side-sheet-beta?
| </div> | ||
|
|
||
| <gux-side-sheet-beta size="medium"> | ||
| <gux-icon slot="icon" icon-name="fa/diamond-regular" size="medium"></gux-icon> |
There was a problem hiding this comment.
Do we want the icon size to be configurable or set by the component?
| display: flex; | ||
| padding: 0 !important; | ||
| background-color: rgb(0 0 0 / 5%) !important; | ||
| } |
There was a problem hiding this comment.
It would be better if this was not in the example file and was part of the core style. I understand that might require bigger changes to the examples app than we want to have in this PR.
| } | ||
|
|
||
| gux-side-sheet-beta { | ||
| flex: 0 0 auto; |
There was a problem hiding this comment.
Is every app going to have to do this or is this specific to the example app?
| @@ -0,0 +1,19 @@ | |||
| :host { | |||
| display: flex; | |||
| gap: 30px; | |||
There was a problem hiding this comment.
This will be over writable in the light dom so you might want to move it into the shadow dom if you don't want devs to be able t change it.
| @@ -0,0 +1,58 @@ | |||
| :host { | |||
| display: block; | |||
There was a problem hiding this comment.
I would have expected this to be absolutely positioned.
We will need to be clear what we are expecting to developers to do with regard to positioning and layout of the component and what we want to have full control over.
| @@ -0,0 +1,39 @@ | |||
| import { Component, Element, h, Prop } from '@stencil/core'; | |||
There was a problem hiding this comment.
This should be in a sub folder
| <slot name="icon" /> | ||
| </div> | ||
| <slot name="heading" /> | ||
| <gux-dismiss-button /> |
There was a problem hiding this comment.
gux-dismiss-button does not have any functionality by default you will have to add a click handler and dismiss functionality
| size: GuxSideSheetSize = 'small'; | ||
|
|
||
| componentWillLoad(): void { | ||
| trackComponent(this.root); |
There was a problem hiding this comment.
we usually track stuff like the size as a varient.
| private root: HTMLElement; | ||
|
|
||
| @Prop() | ||
| size: GuxSideSheetSize = 'small'; |
There was a problem hiding this comment.
If this is just the non modal version I think the sizing is wrong, there should only be small and medium
| <slot name="icon" /> | ||
| </div> | ||
| <slot name="heading" /> | ||
| <gux-dismiss-button /> |
There was a problem hiding this comment.
I dont think the gux-dismiss-button is currently positioned correctly. I think it should be positioned absolute with a top and right of 12px
| <gux-dismiss-button /> | ||
| </header> | ||
| <div class="gux-side-sheet-content"> | ||
| <slot name="content" /> |
There was a problem hiding this comment.
Its not really outlined in the design so probably a UX question but there is an area above the content slot for side-sheet description of some sort. Wondering if there is a need for a description slot.
| var(--gse-ui-dynamicSideSheet-header-paddingRight) | ||
| var(--gse-ui-dynamicSideSheet-header-gap) | ||
| var(--gse-ui-dynamicSideSheet-header-paddingLeft); | ||
| border-bottom: 1px solid var(--gse-ui-modalSideSheet-dividerColor); |
There was a problem hiding this comment.
Token needed here.
| & > footer { | ||
| flex: 0 0 auto; | ||
| padding: var(--gse-ui-dynamicSideSheet-footer-padding); | ||
| border-top: 1px solid var(--gse-ui-modalSideSheet-dividerColor); |
There was a problem hiding this comment.
Token needed here
| render(): JSX.Element { | ||
| return ( | ||
| <this.HeadingTag> | ||
| <slot /> |
There was a problem hiding this comment.
I think we should use a named slot here.
…d/genesys-spark into feature/COMUI-2931-Side_Sheet
|
Hi @conor-darcy whats the status of this component in terms of merging into the gux main library? |
|
@samlimby-genesys just waiting for the current tokens work to be completed first as I need the tokens included there. |
API feedback wanted please for the non-modal version:
https://www.figma.com/design/JKbHmcf4nUF6C7Pj8M6MpY/Spark%3A-Flare-4.0---Core?node-id=28482-782&m=dev