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.

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

feat(ui5-table): adding horizontal column alignment #9228

Closed
wants to merge 1 commit into from

Conversation

nowakdaniel
Copy link
Contributor

Introduction of a new property hAlign. hAlign is used to configure the horizontal alignment in GridCells. The idea is to configure the horizontal alignment on the header level of the GridTable and then automatically adjust the alignment of the cells according to their header cell.

Copy link
Contributor

@aborjinik aborjinik left a comment

Choose a reason for hiding this comment

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

FoA, there is no more Grid but Table you need to solve conflicts.
Also I am not sure if I liked the idea of looping over all cells in every rendering this could be solved with custom css properties.
Another one that I am not sure yet is letting every cell to decide their own alignment?

@@ -31,6 +31,9 @@ abstract class GridCellBase extends UI5Element {
@property({ type: Boolean })
_popin!: boolean;

@property({ type: String, defaultValue: "Left" })
Copy link
Contributor

@aborjinik aborjinik Jun 17, 2024

Choose a reason for hiding this comment

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

type should not be String but Enum

Copy link
Member

Choose a reason for hiding this comment

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

See main/src/types/TableOverflowMode.ts as an example, if you need one

Copy link
Contributor

Choose a reason for hiding this comment

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

this line needs to be adapted after a rebase from main according to #8846

@@ -18,4 +18,23 @@
:host(#selection-cell) {
width: auto;
min-width: auto;
}
:host([h-align="left"]) {
Copy link
Contributor

@aborjinik aborjinik Jun 17, 2024

Choose a reason for hiding this comment

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

I do not get this css here? Why should every cell attribute override the custom css property? Why we need a custom css property here?

Copy link
Member

Choose a reason for hiding this comment

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

Why not just set the css variable in onBeforeRendering of GridCellBase/GridCell? See Grid.ts line 307

this.rows.forEach(row => {
const cell = row.cells[index];
if (cell && cell.hAlign !== header.hAlign) {
cell.hAlign = header.hAlign;
Copy link
Contributor

@aborjinik aborjinik Jun 17, 2024

Choose a reason for hiding this comment

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

so there is a public property in the table cell but we override from the header? Then what is the idea of having a public property on the table cell?

Copy link
Member

@DonkeyCo DonkeyCo left a comment

Choose a reason for hiding this comment

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

Looks promising for now. A couple of things that probably need some refinement I think.
Please consider that the Grid has changed to Table. We need to change that

@@ -31,6 +31,9 @@ abstract class GridCellBase extends UI5Element {
@property({ type: Boolean })
_popin!: boolean;

@property({ type: String, defaultValue: "Left" })
Copy link
Member

Choose a reason for hiding this comment

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

See main/src/types/TableOverflowMode.ts as an example, if you need one

@@ -18,4 +18,23 @@
:host(#selection-cell) {
width: auto;
min-width: auto;
}
:host([h-align="left"]) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just set the css variable in onBeforeRendering of GridCellBase/GridCell? See Grid.ts line 307

@@ -1,6 +1,7 @@
:host {
font-family: var(--sapFontSemiboldDuplexFamily);
color: var(--sapList_HeaderTextColor);
justify-content: var(--_ui5-grid-cell-base-hAlign);
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason, why GridCell has justify-content: var(--_ui5-grid-cell-base-hAlign, left);, while GridHeaderCell has it without left?
Couldnt' this be put into GridCellBase, if we got nearly the same CSS for both cell types?

@ilhan007 ilhan007 changed the title feat(ui5-grid): adding horizontal column alignment feat(ui5-table): adding horizontal column alignment Jun 19, 2024
@github-actions github-actions bot added the Stale label Jul 13, 2024
@github-actions github-actions bot closed this Jul 21, 2024
@nnaydenow nnaydenow deleted the feat-grid-horizontal-alignment branch August 2, 2024 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants