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-grid): implementing new grid component #8362

Merged
merged 93 commits into from
Jun 11, 2024
Merged

feat(ui5-grid): implementing new grid component #8362

merged 93 commits into from
Jun 11, 2024

Conversation

DonkeyCo
Copy link
Member

  • introduces a new table/grid component
  • enhanced accessibility (roles are in the light dom)
  • new API mirroring the structure of the HTML table

DonkeyCo and others added 30 commits February 12, 2024 18:50
Created a GridTable component that currently has no functionality at all.
- included GridTable in bundle file
- add test page GridTable.html
- add bare bones story for GridTable
- create a cell, row and column component

Co-authored-by: Cahit Guerguec <cahit.guerguec@sap.com>
- renamed GridTable and related components to Grid
- created a CSS parameters file and included it in themes
- playground sample for popin and nodata
- instead of using a min-width/min-screen-width as a popin heuristic, use overflow mechanism
- when the table overflows, determines what columns should be moved into the popin
* @default "auto"
* @public
*/
@property({ type: String, defaultValue: "auto" })
Copy link
Member

Choose a reason for hiding this comment

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

We have CSSSize validator available and can be used for width, min/maxWidth props

Copy link
Contributor

@pskelin pskelin May 28, 2024

Choose a reason for hiding this comment

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

CSSSize will be removed, same as typescript accepts string for such properties.

packages/main/src/Grid.ts Outdated Show resolved Hide resolved
packages/main/src/Grid.ts Outdated Show resolved Hide resolved
@ilhan007
Copy link
Member

ilhan007 commented May 28, 2024

*The PR needs to be merged with the latest code in main as there are many unrelated files, shown as changed

Belize is removed

so you can remove any files related to the Belize theme

GridRow

  • We have to check if the key property could be an issue in React. there it's kind of system property as in React all repetitive elements like table rows and list items are forced to have key set for performance reason and as far I remember the key attribute is even not rendered and might cause some problems with the component state.

GridRow / GridSelection

  • I find it odd that I can't just set selected on the ui5-grid-row element, which is usually very convenient when the component like this is used in any framework template.
<ui5-grid>
   <ui5-grid-row selected></ui5-grid-row>
</ui5-grid>
<ui5-grid>
    { 
    this.rows.map((row) => {
          <ui5-grid-row selected={{row.selected}}></ui5-grid-row>
    });
    }
</ui5-grid>

If I got it right, the same can be achieved now via ui5-grid-selection element passing indexes of rows separated with spaces, to its selected property:

<ui5-grid>
   <ui5-grid-row></ui5-grid-row>
   <ui5-grid-selection selected="0 1 2" slot="features"></ui5-grid-selection>
</ui5-grid>
const selectedRowsIndexes = this.rows.map((row, idx) => {
    if (row.selected)  {
         return idx;
    }
}).join(" ");

<ui5-grid>
 { 
    this.rows.map((row) => {
          <ui5-grid-row></ui5-grid-row>
    });
    }
    <ui5-grid-selection selected="{selectedRowsIndexes}" slot="features"></ui5-grid-selection>
</ui5-grid>

Keyboard handling

It's really improved, the cell based navigation is completely new I only noticed that the cell outline overlaps with the content of the cell. I consider it minor, but if it's something easy to fix
Screenshot 2024-05-28 at 17 36 21

Since tag

  • put @since 2.0 on all Grid* classes (at least the public ones)

Futures that are currently missing compared to the current Table,

that can be added later-on if we find it important

  • Grouping of rows,
  • TableRow "navigated"

packages/main/src/Grid.ts Outdated Show resolved Hide resolved
@ilhan007 ilhan007 added the 2.0 label May 29, 2024
@DonkeyCo
Copy link
Member Author

@ilhan007 thanks for the review. Regarding your comments:

*The PR needs to be merged with the latest code in main as there are many unrelated files, shown as changed

Belize is removed

so you can remove any files related to the Belize theme

Should be solved with the upcoming commit.

GridRow

  • We have to check if the key property could be an issue in React. there it's kind of system property as in React all repetitive elements like table rows and list items are forced to have key set for performance reason and as far I remember the key attribute is even not rendered and might cause some problems with the component state.

Yeah. Now that you mention it, it probably won't work. I've seen other components use primaryKey instead of key, maybe this is better? @aborjinik maybe you want to chime in as well?

GridRow / GridSelection

  • I find it odd that I can't just set selected on the ui5-grid-row element, which is usually very convenient when the component like this is used in any framework template.
<ui5-grid>
   <ui5-grid-row selected></ui5-grid-row>
</ui5-grid>
<ui5-grid>
    { 
    this.rows.map((row) => {
          <ui5-grid-row selected={{row.selected}}></ui5-grid-row>
    });
    }
</ui5-grid>

If I got it right, the same can be achieved now via ui5-grid-selection element passing indexes of rows separated with spaces, to its selected property:

<ui5-grid>
   <ui5-grid-row></ui5-grid-row>
   <ui5-grid-selection selected="0 1 2" slot="features"></ui5-grid-selection>
</ui5-grid>
const selectedRowsIndexes = this.rows.map((row, idx) => {
    if (row.selected)  {
         return idx;
    }
}).join(" ");

<ui5-grid>
 { 
    this.rows.map((row) => {
          <ui5-grid-row></ui5-grid-row>
    });
    }
    <ui5-grid-selection selected="{selectedRowsIndexes}" slot="features"></ui5-grid-selection>
</ui5-grid>

I see the use case, where selected directly on the row could be nice, but comparing our selection API to other components, we are not the only ones doing this via a property. Vaadin is also doing this similarly. Boils down to personal preference, but I think it's just nicer to decouple selection from the rows itself and have a central place to query it.

Keyboard handling

It's really improved, the cell based navigation is completely new I only noticed that the cell outline overlaps with the content of the cell. I consider it minor, but if it's something easy to fix Screenshot 2024-05-28 at 17 36 21

Will have a look at this issue.

Since tag

  • put @since 2.0 on all Grid* classes (at least the public ones)

Fair point. I'll add that in the upcoming commit.

Futures that are currently missing compared to the current Table,

that can be added later-on if we find it important

  • Grouping of rows,
  • TableRow "navigated"

We have a BLI already for navigation indication (as well as row actions), grouping is not on the list yet, but I'll bring it up with the colleagues.

- remove unnecessary resources folder for test pages
- adjust bundle imports
- add 2.0 version
@DonkeyCo DonkeyCo requested a review from ilhan007 May 29, 2024 13:32
@DonkeyCo DonkeyCo merged commit 04d291d into main Jun 11, 2024
10 checks passed
@ilhan007 ilhan007 deleted the feat-new-table branch June 11, 2024 07:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants