ui5-webcomponents icon indicating copy to clipboard operation
ui5-webcomponents copied to clipboard

feat(ui5-grid): implementing new grid component

Open DonkeyCo opened this issue 1 year ago • 1 comments

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

DonkeyCo avatar Feb 28 '24 08:02 DonkeyCo

Hi @DonkeyCo row-click instead of row-press is also more consistent with the existing components and events.

ilhan007 avatar May 15 '24 11:05 ilhan007

*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"

ilhan007 avatar May 28 '24 14:05 ilhan007

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

DonkeyCo avatar May 29 '24 12:05 DonkeyCo