fix(ui5-table): adds the missing TableHeaderRow dependency #9490
Conversation
aborjinik
commented
Jul 16, 2024
•
aborjinik
commented
- The missing TableHeaderRow dependency is added
- Busy indicator dim background is added
- Explicit header row height is removed
- Multiple selection mode sample is fixed
- Navigated outline is aligned with spec
| @@ -39,4 +39,8 @@ | |||
| inset: 0; | |||
| height: 100%; | |||
| z-index: 2; | |||
| } | |||
|
|
|||
| #loading[_is-busy] { | |||
There was a problem hiding this comment.
@nnaydenow shouldn't this be handled by the busy indicator itself?
it seems like the following lines expects busy indicator to be rendered as parent of the component but we render it as a child of the table. If you guys wants change it please let me know the I can remove it. I am not happy writing a style for the private attribute of another component.
https://github.com/SAP/ui5-webcomponents/blob/main/packages/main/src/themes/BusyIndicator.css#L98-L100
There was a problem hiding this comment.
+1
Also not a fan of this, it would be good if we could avoid this somehow.
Is is not feasible to check BusyIndicator.active instead? Or are there issues with rendering then?
There was a problem hiding this comment.
I simply expect BusyIndicator adds its own dim background color, independent from whether it is rendered as a child or a wrapper.
| @@ -12,7 +12,7 @@ | |||
| <div class="section"> | |||
| <!-- playground-fold-end --> | |||
| <div id="selectionGroup" role="radiogroup"> | |||
| <ui5-radio-button name="selection" text="Multi" checked></ui5-radio-button> | |||
| <ui5-radio-button name="selection" text="Multiple" checked></ui5-radio-button> | |||
There was a problem hiding this comment.
@ilhan007 switching selection mode was working fine but it seems like we forgot to change to the new convention value in the test page.
@pskelin this is a good example not to fallback to the default value of enums, so +1 to
#8846 (comment)
| @@ -160,6 +160,7 @@ type TableRowClickEventDetail = { | |||
| fastNavigation: true, | |||
| dependencies: [ | |||
| BusyIndicator, | |||
| TableHeaderRow, | |||
There was a problem hiding this comment.
@ilhan007 I am still not 100% sure about this change. We are not rendering TableHeaderRow in our hbs file but a table without the header row is not a table since it is required to define the columns of the table.
Technically it is not a dependency but I am not sure whether we should always consider in our implementation that headerRow[0] might be undefined. In every sample I checked the headerRow[0] was defined even in onEnterDom for the following structure but not in the playground
<ui5-table>
<ui5-header-row slot="headerRow">
Might this cause any cyclic dependency?
There was a problem hiding this comment.
Cyclic dependency should not occur here, if we import TableHeaderRow, at least from what I can see.
There is no reference import to Table.ts in TableHeaderRow.ts, TableHeaderCell.ts and TableRowBase.ts.
To be sure, you can also try to build the project locally and use the build in a custom project.
aborjinik
force-pushed
the
table-dependency-fixes
branch
2 times, most recently
from
9eb9f2b to
933cef9
Compare
- Busy indicator dim background is added - Explicit header row height is removed - Multiple selection mode sample is fixed - Navigated outline is aligned with spec
aborjinik
force-pushed
the
table-dependency-fixes
branch
from
933cef9 to
af337ef
Compare
| @@ -160,6 +160,7 @@ type TableRowClickEventDetail = { | |||
| fastNavigation: true, | |||
| dependencies: [ | |||
| BusyIndicator, | |||
| TableHeaderRow, | |||
There was a problem hiding this comment.
Cyclic dependency should not occur here, if we import TableHeaderRow, at least from what I can see.
There is no reference import to Table.ts in TableHeaderRow.ts, TableHeaderCell.ts and TableRowBase.ts.
To be sure, you can also try to build the project locally and use the build in a custom project.
| @@ -39,4 +39,8 @@ | |||
| inset: 0; | |||
| height: 100%; | |||
| z-index: 2; | |||
| } | |||
|
|
|||
| #loading[_is-busy] { | |||
There was a problem hiding this comment.
+1
Also not a fan of this, it would be good if we could avoid this somehow.
Is is not feasible to check BusyIndicator.active instead? Or are there issues with rendering then?
