Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ui5-table): adds the missing TableHeaderRow dependency #9490

Merged
merged 1 commit into from
Jul 23, 2024
+32 −11
Merged

fix(ui5-table): adds the missing TableHeaderRow dependency #9490

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/main/src/Table.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ import { getI18nBundle } from "@ui5/webcomponents-base/dist/i18nBundle.js";
import TableTemplate from "./generated/templates/TableTemplate.lit.js";
import TableStyles from "./generated/themes/Table.css.js";
import TableRow from "./TableRow.js";
import type TableHeaderRow from "./TableHeaderRow.js";
import TableHeaderRow from "./TableHeaderRow.js";
import type TableHeaderCell from "./TableHeaderCell.js";
import TableExtension from "./TableExtension.js";
import type TableSelection from "./TableSelection.js";
Expand Down Expand Up @@ -160,6 +160,7 @@ type TableRowClickEventDetail = {
fastNavigation: true,
dependencies: [
BusyIndicator,
TableHeaderRow,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@aborjinik aborjinik Jul 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

@ilhan007 you mentioned that you are discussing with @pskelin. For now I submit the change as having the TableHeaderRow as dependency but if you figure out something please let us know since this dependency IMHO is not a real dependency.

TableCell,
TableRow,
],
Expand Down
4 changes: 3 additions & 1 deletion packages/main/src/TableRow.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,9 @@
{{/each}}

{{#if _renderNavigated}}
<ui5-table-cell id="navigated-cell" excluded-from-navigation></ui5-table-cell>
<ui5-table-cell id="navigated-cell" excluded-from-navigation>
<div id="navigated"></div>
</ui5-table-cell>
{{/if}}

{{#if _popinCells.length}}
Expand Down
4 changes: 4 additions & 0 deletions packages/main/src/themes/Table.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,4 +39,8 @@
inset: 0;
height: 100%;
z-index: 2;
}

#loading[_is-busy] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I simply expect BusyIndicator adds its own dim background color, independent from whether it is rendered as a child or a wrapper.

background-color: var(--_ui5_busy_indicator_block_layer);
}
1 change: 0 additions & 1 deletion packages/main/src/themes/TableHeaderRow.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
:host {
background: var(--sapList_HeaderBackground);
grid-template-rows: var(--_ui5_list_item_base_height);
DonkeyCo marked this conversation as resolved.
Show resolved Hide resolved
border-bottom: var(--sapList_BorderWidth) solid var(--sapList_HeaderBorderColor);
}

Expand Down
16 changes: 15 additions & 1 deletion packages/main/src/themes/TableRow.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,15 +34,29 @@
}

#navigated-cell {
position: relative;
overflow: visible;
grid-row: span 2;
min-width: 0;
padding: 0;
}

:host([navigated]) #navigated-cell {
:host([navigated]) #navigated {
position: absolute;
inset: 0;
background-color: var(--sapList_SelectionBorderColor);
}

:host([tabindex]:focus) #navigated {
transform: translateX(calc(var(--_ui5_table_navigated_cell_width) * -1));
bottom: 2px;
top: 3px;
}

:host([tabindex]:focus) #navigated:dir(rtl) {
transform: translateX(var(--_ui5_table_navigated_cell_width));
}

:host([navigated]) #popin-cell {
grid-column: 1 / -2;
}
2 changes: 1 addition & 1 deletion packages/main/src/themes/base/Table-parameters.css
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
:root {
--_ui5_table_cell_valign: center;
--_ui5_table_cell_min_width: 2.75rem;
--_ui5_table_navigated_cell_width: 0.25rem;
--_ui5_table_navigated_cell_width: 0.1875rem;
}
10 changes: 5 additions & 5 deletions packages/main/test/specs/Table.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,13 +220,13 @@ describe("Table - Navigated Rows", async () => {
assert.strictEqual(await navigatedCell1.getAttribute("excluded-from-navigation"), "", "The navigated cell is excluded from item navigation");
assert.strictEqual(await navigatedCell2.getAttribute("excluded-from-navigation"), "", "The navigated cell is excluded from item navigation");

const navigatedCell1BG = await browser.executeAsync(done => {
done(getComputedStyle(document.getElementById("row1").shadowRoot.querySelector("#navigated-cell")).backgroundColor);
const navigated1BG = await browser.executeAsync(done => {
done(getComputedStyle(document.getElementById("row1").shadowRoot.querySelector("#navigated")).backgroundColor);
});
const navigatedCell2BG = await browser.executeAsync(done => {
done(getComputedStyle(document.getElementById("row2").shadowRoot.querySelector("#navigated-cell")).backgroundColor);
const navigated2BG = await browser.executeAsync(done => {
done(getComputedStyle(document.getElementById("row2").shadowRoot.querySelector("#navigated")).backgroundColor);
});
assert.notEqual(navigatedCell1BG, navigatedCell2BG, "Background color of navigated cell is different from the one of non-navigated cell");
assert.notEqual(navigated1BG, navigated2BG, "Background color of navigated cell is different from the one of non-navigated cell");

const gridTemplateColumns = await browser.executeAsync(done => {
done(document.getElementById("table1").shadowRoot.querySelector("#table").style.gridTemplateColumns);
Expand Down
1 change: 1 addition & 0 deletions packages/website/docs/_samples/main/Table/Selection/main.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import "@ui5/webcomponents/dist/Table.js";
import "@ui5/webcomponents/dist/TableHeaderRow.js";
import "@ui5/webcomponents/dist/TableHeaderCell.js";
import "@ui5/webcomponents/dist/TableSelection.js";
import "@ui5/webcomponents/dist/Label.js";
import "@ui5/webcomponents/dist/Input.js";
import "@ui5/webcomponents/dist/ComboBox.js";
Expand Down
2 changes: 1 addition & 1 deletion packages/website/docs/_samples/main/Table/Selection/sample.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

<ui5-radio-button name="selection" text="Single"></ui5-radio-button>
<ui5-radio-button name="selection" text="None"></ui5-radio-button>
</div>
Expand Down