feat(list-item): introduce new styles#1949
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the List Item component, including its documentation, SCSS styles, and interactive examples. The feedback highlights several areas for improvement: removing placeholder suffixes in documentation headings, increasing CSS selector specificity in the theme styles, and adhering to UX writing guidelines regarding timestamp vocabulary, telegram-style descriptions, button placement, and label capitalization.
c072124 to
8d5328e
Compare
|
@spike-rabbit could you take a look at the example and provide some initial feedback if the structure looks as expected? |
panch1739
left a comment
There was a problem hiding this comment.
@ljanner Looks great!
Just one observation...in the list-item-actions, the active/selected state seems to be missing. When the list item is pressed, the background color should change to base-1-selected.
Questions:
- The replacement of the notification item would happen in another PR?
- Does this have the "unread" property also? (This is only valid for notification item)
f64852f to
7ab5d4b
Compare
I now added a new example called list-item-unread. While an unread state isn't quite generic I do think it makes sense to provide it since the idea is to drop the notification item entirely.
For the replacement itself, we need to choose a technical solution either:
This is still open, we can see how far we can get with migrations for element v50. For now I added a banner on the notification item docs recommending list-item instead. |
@ljanner Excellent! Is good for me :) Thank you very much! |
1aac650 to
f75d501
Compare
review status of pr seems bugged
DEPRECATED: `SiNotificationItemComponent` is deprecated. Use the CSS-based list item component instead. Migrate by replacing `<si-notification-item>` with the `.list-item` CSS classes applied to semantic HTML elements. See the list item documentation for usage examples and accessibility guidance.
| <li class="list-item"> | ||
| <time class="list-item-timestamp" datetime="09:30">Today at 09:30</time> | ||
| <h5 class="list-item-title">Firmware update pending approval</h5> | ||
| <div class="list-item-primary-action"> |
There was a problem hiding this comment.
I am not exactly in favor of the name primary-action.
The UX specs only show a menu there. So what about calling it list-item-menu?
There was a problem hiding this comment.
I would go something like context-action, the position is at the end and its doesn't need to be a menu it could also be on or more action buttons.
Just a side question, should we shorten the naming?
.list-item:
- .title
- .timestamp
- .description
- .metadata
There was a problem hiding this comment.
The naming here is difficult and has previously also been a discussion for the notification-item. Here is the link to the full discussion (internal): https://code.siemens.com/simpl/simpl-element/-/work_items/2371#note_39626788
I also think list-item-menu is too specific as other action buttons are totally valid. To me personally the name context-action lacks hierarchal meaning.
I'm open to something like list-item-main-action or list-item-inline-action.
/cc @kfenner
Resolves #1641
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: