feat(list-item): add iconStart, iconEnd and iconFlipRtl props #11004
Conversation
aPreciado88
added
the
pr ready for visual snapshots
|
@ashetland @SkyeSeitz Can you please take a look at the new screenshots? |
…ciado88/9175-list-item-add-icon-end-icon-start-props
aPreciado88
added
pr ready for visual snapshots
There was a problem hiding this comment.
Update list.html and list.stories.ts to use icon-start and icon-end instead of content slots.
Can we leave the examples using slots since using slots is still a valid use case and can track the ordering of content slots and icon props.
All other changes look good!
Just had a question on whether our design consistency puts content slots outside or inside icon props.
| @@ -142,18 +174,6 @@ export const scales = (): string => html` | |||
| scale="s" | |||
| slot="actions-start" | |||
| ></calcite-action> | |||
| <calcite-icon | |||
There was a problem hiding this comment.
@aPreciado88 lets leave these in the demo since it is still valid to use the slots.
There was a problem hiding this comment.
@driskull I chatted with Aaron about this and the suggestion was to get rid of content slots in favor of iconStart and iconEnd. Should I add all of the content slots back or only add them to a couple of tests?
cc @ashetland
There was a problem hiding this comment.
We could also use calcite-avatar in the content slots. Since we now have icon-start/end it felt best to move icons to the props, leaving the content slots open for custom content.
There was a problem hiding this comment.
Yeah we can update them to use an avatar if we want. My point is that they are still valid slots and users can use both icon props and content slots.
There was a problem hiding this comment.
Honestly, we removed a lot of them to try to get that story to load faster, but I believe we were going to log a separate issue to clean up the List stories. This story in particular seems to always show a diff because the screenshot is taken before the page finishes loading.
| @@ -919,7 +957,9 @@ export class ListItem | |||
| const content = [ | |||
| this.renderContentStart(), | |||
| this.renderCustomContent(), | |||
| this.renderIconStart(), | |||
There was a problem hiding this comment.
Question: Do we always put the icons inside the custom content slots or vice versa? Just want to confirm for designs. cc @ashetland @SkyeSeitz
There was a problem hiding this comment.
Now that we have the icon props, we should be using them for Calcite icons. The content slots can be used for other custom content.
There was a problem hiding this comment.
Correct. my question was whether the custom content like an avatar surrounds the icons or goes inside of them.
There was a problem hiding this comment.
Sorry for the confusion. The content slots should be on the outside of the icons:
There was a problem hiding this comment.
Sounds good. Code looks correct for that 👍
…ciado88/9175-list-item-add-icon-end-icon-start-props
…ciado88/9175-list-item-add-icon-end-icon-start-props
aPreciado88
added
pr ready for visual snapshots
|
@driskull @ashetland I added back the content slots and updated them to use |
aPreciado88
deleted the
aPreciado88/9175-list-item-add-icon-end-icon-start-props
branch
**Related Issue:** [#9175](#9175) ## Summary - Add `iconStart`, `iconEnd` and `iconFlipRtl` props. - Update default icon color to `--calcite-color-text-3`. - Update icon color to `--calcite-color-text-1` when item is selected. - Update `list.html` and `list.stories.ts` to use `icon-start` and `icon-end`.
Related Issue: #9175
Summary
iconStart,iconEndandiconFlipRtlprops.--calcite-color-text-3.--calcite-color-text-1when item is selected.list.htmlandlist.stories.tsto useicon-startandicon-end.