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

Jump to bottom

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

Merged
merged 50 commits into from
Sep 30, 2024
Merged

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

merged 50 commits into from
Sep 30, 2024

Conversation

nowakdaniel
Copy link
Contributor

@nowakdaniel nowakdaniel commented Aug 5, 2024 •
edited
Loading

Introduction of a new property hAlign. hAlign is used to configure the horizontal alignment. 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.

Tests are still missing

@ilhan007 ilhan007 changed the title feat(ui5-grid): adding horizontal column alignment feat(ui5-table): adding horizontal column alignment Aug 5, 2024
@nowakdaniel nowakdaniel force-pushed the feat-grid-horizontal-alignment branch from 7b833aa to 2a4c627 Compare August 5, 2024 13:40
DonkeyCo
DonkeyCo requested changes Aug 7, 2024
View reviewed changes
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.

As already mentioned by you, tests are missing. Make sure to test, if changing the value and setting it after initial rendering actually works as expected.

Please also add the necessary documentation as well as a sample, that can be shown in the documentation/demokit.

packages/main/src/Table.ts Outdated Show resolved Hide resolved
packages/main/src/Table.ts Outdated Show resolved Hide resolved
packages/main/src/types/TableCellAlign.ts Outdated Show resolved Hide resolved
packages/main/test/pages/Table.html Outdated Show resolved Hide resolved
packages/main/test/pages/Table.html Outdated Show resolved Hide resolved
packages/main/src/types/TableCellAlign.ts Outdated Show resolved Hide resolved
packages/main/src/TableCellBase.ts Outdated Show resolved Hide resolved
DonkeyCo
DonkeyCo requested changes Aug 23, 2024
View reviewed changes
packages/main/src/Table.ts Outdated Show resolved Hide resolved
packages/main/src/TableCellBase.ts Outdated Show resolved Hide resolved
packages/main/src/TableCellBase.ts Show resolved Hide resolved
packages/main/src/TableCellBase.ts Outdated Show resolved Hide resolved
packages/main/src/Table.ts Outdated Show resolved Hide resolved
packages/main/src/types/TableCellHorizontalAlign.ts Outdated Show resolved Hide resolved
packages/main/test/pages/HAlignTable.html Outdated Show resolved Hide resolved
aborjinik
aborjinik reviewed Sep 23, 2024
View reviewed changes
packages/main/src/TableHeaderCell.ts Outdated
@@ -78,13 +78,20 @@ class TableHeaderCell extends TableCellBase {

protected ariaRole: string = "columnheader";
_popinWidth: number = 0;
_individualSlot?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

do we really need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I remove this I get warnings.
Property '_individualSlot' does not exist on type 'TableHeaderCell'.

See #9639 (comment) for more information.
@DonkeyCo

Copy link
Member

Choose a reason for hiding this comment

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

This is just needed for TS to be happy and avoid warnings as Daniel mentioned

Copy link
Contributor

Choose a reason for hiding this comment

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

then this should better be in the CellBase

ivoplashkov and others added 9 commits September 25, 2024 14:48
@ivoplashkov @nowakdaniel
docs(ui5-page): correct noScrolling documentation (#9925)
95048ad 
FIXES: #9840
@pskelin @nowakdaniel
fix(ui5-barcode-scanner-dialog): can be imported in server environmen…
5698c65 
…ts with a shim (#9901)
@pskelin @nowakdaniel
chore: update chromedriver 128 (#9926)
9a59950
@kineticjs @nowakdaniel
chore: simplify guided prompt sample for dialog (#9903)
df59420
@dependabot @nowakdaniel
chore(deps): bump rollup from 3.28.1 to 3.29.5 (#9928)
e3d4e45 
Bumps [rollup](https://github.com/rollup/rollup) from 3.28.1 to 3.29.5.
- [Release notes](https://github.com/rollup/rollup/releases)
- [Changelog](https://github.com/rollup/rollup/blob/master/CHANGELOG.md)
- [Commits](rollup/rollup@v3.28.1...v3.29.5)

---
updated-dependencies:
- dependency-name: rollup
  dependency-type: indirect
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@dependabot @nowakdaniel
chore(deps): bump vite from 4.5.3 to 4.5.5 (#9889)
d8bc9ed 
Bumps [vite](https://github.com/vitejs/vite/tree/HEAD/packages/vite) from 4.5.3 to 4.5.5.
- [Release notes](https://github.com/vitejs/vite/releases)
- [Changelog](https://github.com/vitejs/vite/blob/v4.5.5/packages/vite/CHANGELOG.md)
- [Commits](https://github.com/vitejs/vite/commits/v4.5.5/packages/vite)

---
updated-dependencies:
- dependency-name: vite
  dependency-type: direct:production
...

Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@ilhan007 @nowakdaniel
docs: enhance Table docs (#9929)
2810252 
Fixes: #9920
@MapTo0 @nowakdaniel
fix(ui5-page): fix double footer and adjust animation timing (#9885)
e901877 
FIXES: #9755
FIXES: #9756
@nowakdaniel
feat(ui5-table): adjust jsdoc of horizontalAlign property
65a0bde
DonkeyCo
DonkeyCo reviewed Sep 25, 2024
View reviewed changes
packages/main/src/TableCell.ts Outdated Show resolved Hide resolved
packages/main/src/TableHeaderCell.ts Outdated
@@ -78,13 +78,20 @@ class TableHeaderCell extends TableCellBase {

protected ariaRole: string = "columnheader";
_popinWidth: number = 0;
_individualSlot?: string;
Copy link
Member

Choose a reason for hiding this comment

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

This is just needed for TS to be happy and avoid warnings as Daniel mentioned

aborjinik
aborjinik approved these changes Sep 26, 2024
View reviewed changes
packages/main/src/TableHeaderCell.ts Outdated
@@ -78,13 +78,20 @@ class TableHeaderCell extends TableCellBase {

protected ariaRole: string = "columnheader";
_popinWidth: number = 0;
_individualSlot?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

then this should better be in the CellBase

packages/main/src/TableCellBase.ts Show resolved Hide resolved
@nowakdaniel nowakdaniel merged commit 65f75eb into main Sep 30, 2024
10 checks passed
@DonkeyCo DonkeyCo deleted the feat-grid-horizontal-alignment branch September 30, 2024 12:46
@nowakdaniel nowakdaniel mentioned this pull request Oct 16, 2024
Merged
nowakdaniel added a commit that referenced this pull request Oct 18, 2024
@nowakdaniel
fix(ui5-table): wrong horiz. alignm. behavior and wrong texts (#10040)
f3da992 
Fixed a problem where the horizontalAlign property would be applied to cells inside the popin area.
In addition I've changed some of the texts reviewed in the last PR (#9639).

fixes: #10017
kgogov pushed a commit that referenced this pull request Oct 30, 2024
@nowakdaniel @kgogov
fix(ui5-table): wrong horiz. alignm. behavior and wrong texts (#10040)
8c45810 
Fixed a problem where the horizontalAlign property would be applied to cells inside the popin area.
In addition I've changed some of the texts reviewed in the last PR (#9639).

fixes: #10017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pskelin pskelin pskelin left review comments

@KvM2 KvM2 KvM2 left review comments

@aborjinik aborjinik aborjinik approved these changes

@DonkeyCo DonkeyCo DonkeyCo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.