Skip to content

feat(side-panel): extract status actions into new component#1676

Open
spike-rabbit wants to merge 1 commit intomainfrom
feat/extract-side-panel-status-actions
Open

feat(side-panel): extract status actions into new component#1676
spike-rabbit wants to merge 1 commit intomainfrom
feat/extract-side-panel-status-actions

Conversation

@spike-rabbit
Copy link
Member

@spike-rabbit spike-rabbit commented Mar 18, 2026

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:

<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>

Documentation.
Examples.
Dashboards Demo.
Playwright report.

Coverage Reports:

Code Coverage

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@spike-rabbit spike-rabbit force-pushed the feat/extract-side-panel-status-actions branch 3 times, most recently from 5dd4e5e to d367492 Compare March 18, 2026 16:55
@spike-rabbit spike-rabbit marked this pull request as ready for review March 18, 2026 17:09
@spike-rabbit spike-rabbit requested review from a team as code owners March 18, 2026 17:09
Copy link
Member

@spliffone spliffone left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@spike-rabbit spike-rabbit force-pushed the feat/extract-side-panel-status-actions branch 2 times, most recently from 4e7ca76 to 846a27d Compare March 19, 2026 06:46
@spike-rabbit
Copy link
Member Author

@spliffone I chose a component here to maintain some flexibility for us in the future.
They way this currently behaves based on expanded / collapsed of the side-panel is not ideal.
I am afraid, that fixing this may requires JS logic.

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).
So I am targetting to get rid of this component completely.

But currently, the main focus is solve a consumer request.

@spike-rabbit spike-rabbit force-pushed the feat/extract-side-panel-status-actions branch from 846a27d to 4512f3f Compare March 19, 2026 07:08
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>
```
@spike-rabbit spike-rabbit force-pushed the feat/extract-side-panel-status-actions branch from 4512f3f to 32c49c0 Compare March 19, 2026 07:54
Copy link
Member

@ljanner ljanner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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&#10;active, ack</span>
Copy link
Member

@ljanner ljanner Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would think it would be more clear if a to use a line break like so:

Suggested change
<span class="text-pre-wrap">Event source&#10;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;
}
Copy link
Member

@ljanner ljanner Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Required for https://github.com/siemens/element/pull/1676/changes#r2964815774

Suggested change
}
span {
color: all-variables.$element-text-primary;
}

Comment on lines +14 to +15
inline-size: 24px;
line-height: 24px;
Copy link
Member

@ljanner ljanner Mar 20, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Suggested change
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">
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<span class="ms-2 auto-hide text-start">
<span class="ms-2 auto-hide si-caption text-start">

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants