feat(side-panel): extract status actions into new component#1676
feat(side-panel): extract status actions into new component#1676spike-rabbit wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request is a great improvement, extracting the status actions into new standalone components. This refactoring simplifies the API for developers and allows for more flexible use of features like overlays, while also deprecating the older StatusItem interface. The implementation largely adheres to the repository's style guide. I've identified a minor opportunity for improvement in the SCSS to avoid using !important, which will enhance maintainability.
5dd4e5e to
d367492
Compare
spliffone
left a comment
There was a problem hiding this comment.
I would prefer an approach where we just use content-projection and some CSS classes to show status actions which offers more flexibility for consumers. The status action components has simply to much logic inside.
projects/element-ng/side-panel/si-side-panel-action.component.ts
Outdated
Show resolved
Hide resolved
4e7ca76 to
846a27d
Compare
|
@spliffone I chose a component here to maintain some flexibility for us in the future. On the long run, I would like to have a more flexible solution to render icons / actions in a collapsed side-panel (needed for AI patterns). But currently, the main focus is solve a consumer request. |
846a27d to
4512f3f
Compare
The main motivation is to allow access to the native elements.
In some applications a status action triggers a menu or overlay.
This is currently built by those apps using a DOM query.
With this change, they can just use the overlay directive.
In addition, we can deprecate the `StatusItem` interface,
which depends on the already deprecated `MenuItem`.
The usage is quite simple:
```html
<si-side-panel-content>
<si-side-panel-actions>
<button
type="button"
si-side-panel-action
icon="element-alarm-background-filled"
iconColor="status-danger"
overlayIcon="element-alarm-tick"
overlayIconColor="text-body"
(click)="action()"
>
Action
</button>
</si-side-panel-actions>
</si-side-panel-content>
```
4512f3f to
32c49c0
Compare
ljanner
left a comment
There was a problem hiding this comment.
@spike-rabbit great changes! I really like the clear naming and good examples.
Just two small details. The commit body already gives great insight to the change, but please add a deprecation note so it properly shows up in the changelog.
| overlayIconColor="text-body" | ||
| (click)="logEvent('Event source, active, ack')" | ||
| > | ||
| <span class="text-pre-wrap">Event source active, ack</span> |
There was a problem hiding this comment.
I would think it would be more clear if a to use a line break like so:
| <span class="text-pre-wrap">Event source active, ack</span> | |
| <span>Event source<br>active, ack</span> |
Not fully sure what the recommended way is.
| span { | ||
| font-size: 12px; | ||
| color: all-variables.$element-text-primary; | ||
| } |
There was a problem hiding this comment.
Required for https://github.com/siemens/element/pull/1676/changes#r2964815774
| } | |
| span { | |
| color: all-variables.$element-text-primary; | |
| } |
| inline-size: 24px; | ||
| line-height: 24px; |
There was a problem hiding this comment.
Since the dot is displayed when the action is disabled and the action consist of the icon mainly this should probably replicate the icon-size that used to be 24px AFAIK and wasn't updated most likely, therefore we should keep the separator aligned with the icon size imo
| inline-size: 24px; | |
| line-height: 24px; | |
| inline-size: all-variables.$si-icon-font-size; | |
| line-height: all-variables.$si-icon-font-size; |
| @if (overlayIcon(); as overlayIcon) { | ||
| <si-icon class="icon position-absolute" [class]="overlayIconColor()" [icon]="overlayIcon" /> | ||
| } | ||
| <span class="ms-2 auto-hide text-start"> |
There was a problem hiding this comment.
| <span class="ms-2 auto-hide text-start"> | |
| <span class="ms-2 auto-hide si-caption text-start"> |
The main motivation is to allow access to the native elements.
In some applications a status action triggers a menu or overlay.
This is currently built by those apps using a DOM query.
With this change, they can just use the overlay directive.
In addition, we can deprecate the
StatusIteminterface,which depends on the already deprecated
MenuItem.The usage is quite simple:
Documentation.
Examples.
Dashboards Demo.
Playwright report.
Coverage Reports: