Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature by allowing a custom toolbar within the si-tabset component. The implementation is clean, particularly the use of :empty to hide the toolbar when it's not in use. However, the new example code has a couple of issues that should be addressed, and the new functionality lacks test coverage. I've provided specific comments on the example file to correct the tab selection logic and to add visual content to the toolbar buttons. Additionally, I recommend adding automated tests to si-tabset.component.spec.ts to ensure the toolbar's content projection works as expected and to prevent future regressions.
9e27e46 to
d639fad
Compare
|
Documentation. Coverage Reports: |
d639fad to
e01a841
Compare
| <si-icon class="icon m-0" [icon]="icons.elementOptions" /> | ||
| </button> | ||
| } | ||
| <div class="tab-toolbar"> |
There was a problem hiding this comment.
@spliffone are there alternatives that make it work like this:
<div class="tab-toolbar">
<si-tabset >... </si-tabset>
<div class="toolbar">
<button class="..."></button>
</div>
</div>
I don't like having them as a content projection. It looks super weird to have non tabs in a tabset.
This also prevents us to do potential clean up in the DOM which results in us moving the the tablist role to root element, to allow custom labels etc.
There was a problem hiding this comment.
I disagree here, the tab order should ensure that actions are focusable before the actual content. Following your proposal means the content is before the actions. Can you further explain what do we plan to clean up, I am uncertain what you mean?
1234c2f to
3aa7ed3
Compare
3aa7ed3 to
94fad3d
Compare
Uh oh!
There was an error while loading. Please reload this page.