Skip to content

feat(ui5-table-group-row): introduce new component #3470

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
Merged
merged 6 commits into from
Jun 29, 2021
Merged

feat(ui5-table-group-row): introduce new component #3470

merged 6 commits into from
Jun 29, 2021

Conversation

hristop
Copy link
Contributor

@hristop hristop commented Jun 25, 2021 •
edited by fifoosid
Loading

The new type of row represents group header row which can be used in case a grouping in the table is required.

Fixes #3319

@fifoosid fifoosid changed the title feat(ui5-table-group-row): New table row type to allow grouping feat(ui5-table-group-row): introduce new component Jun 25, 2021
@fifoosid fifoosid closed this Jun 25, 2021
@fifoosid fifoosid reopened this Jun 25, 2021
@hristop hristop requested a review from simonarangelova June 25, 2021 08:09
fifoosid
fifoosid suggested changes Jun 25, 2021
View reviewed changes
Copy link
Contributor

@fifoosid fifoosid left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please add a sample in the Table.sample.html
Add TableGroupRow to the appenddocs tag in Table.js

packages/main/src/TableGroupRow.js Outdated
* Defines the mode of the row (None, SingleSelect, MultiSelect).
* @type {TableMode}
* @defaultvalue "None"
* @since 1.0.0-rc.15
Copy link
Contributor

Choose a reason for hiding this comment

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

No since tag is needed here as the whole component is available since this version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

packages/main/src/TableGroupRow.js
* @author SAP SE
* @alias sap.ui.webcomponents.main.TableGroupRow
* @extends sap.ui.webcomponents.base.UI5Element
* @tagname ui5-table-group-row
Copy link
Contributor

Choose a reason for hiding this comment

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

Add the since tag here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

1.0.0-rc.15

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

packages/main/src/themes/TableGroupRow.css Outdated
.ui5-table-group-row-root:focus {
outline: var(--ui5_table_row_outline_width) dotted var(--sapContent_FocusColor);
outline-offset: -0.0625rem;

Copy link
Contributor

Choose a reason for hiding this comment

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

delete blank line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

simonarangelova
simonarangelova reviewed Jun 25, 2021
View reviewed changes
packages/main/src/TableGroupRow.js Outdated
* <br>
* The <code>ui5-table-group-row</code> exposes the following CSS Shadow Parts:
* <ul>
* <li>group-row - Used to style the native <code>tr</code> element</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

I would add tag

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

packages/main/src/TableGroupRow.js Outdated
* <br>
* The <code>ui5-table-group-row</code> exposes the following CSS Shadow Parts:
* <ul>
* <li>group-row - Used to style the native <code>tr</code> element</li>
Copy link
Contributor

Choose a reason for hiding this comment

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

element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@hristop
Copy link
Contributor Author

hristop commented Jun 25, 2021

Please add a sample in the Table.sample.html
Add TableGroupRow to the appenddocs tag in Table.js

Both were added.

fifoosid
fifoosid suggested changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@fifoosid fifoosid left a comment •
edited
Loading

Choose a reason for hiding this comment

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

A couple of small things left

packages/main/src/TableGroupRow.js
* @author SAP SE
* @alias sap.ui.webcomponents.main.TableGroupRow
* @extends sap.ui.webcomponents.base.UI5Element
* @tagname ui5-table-group-row
Copy link
Contributor

Choose a reason for hiding this comment

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

1.0.0-rc.15

packages/main/test/pages/TableGrouping.html Outdated

<ui5-table-group-row>Country: Bulgaria</ui5-table-group-row>

<ui5-table-row data-city="Sofia">
Copy link
Contributor

Choose a reason for hiding this comment

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

data-city attribute is not needed anywhere

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

packages/main/test/samples/Table.sample.html Outdated

<ui5-table-group-row>Country: Bulgaria</ui5-table-group-row>

<ui5-table-row data-city="Sofia">
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@fifoosid fifoosid requested a review from a team June 28, 2021 09:38
ilhan007
ilhan007 reviewed Jun 28, 2021
View reviewed changes
packages/main/src/TableGroupRow.js
},
},
properties: /** @lends sap.ui.webcomponents.main.TableGroupRow.prototype */ {
/**
Copy link
Member

Choose a reason for hiding this comment

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

Does the GroupRow support selection modes? If not probably you can remove this prop.
Or it's done because of the code in Table.js

 449: row.mode = this.mode;

Assigning the rows' "mode" field by the Table should not fail if the GroupRow does not have mode. And furthermore, it won't be re-rendered when the selection mode is change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The mode is needed as we need to know the exact number of columns, which is +1 in case of multi select mode.

Copy link
Member

Choose a reason for hiding this comment

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

If you need it, then it's fine, thanks

@hristop hristop requested a review from fifoosid June 28, 2021 10:33
fifoosid
fifoosid suggested changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@fifoosid fifoosid left a comment

Choose a reason for hiding this comment

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

In TableGrouping.html there are a couple more occurrences of data-city attribute

@hristop hristop requested a review from fifoosid June 28, 2021 10:44
fifoosid
fifoosid approved these changes Jun 28, 2021
View reviewed changes
Copy link
Contributor

@fifoosid fifoosid left a comment

Choose a reason for hiding this comment

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

LGTM, when someone from @SAP/ui5-webcomponents-topic-rl approves, we can merge

MapTo0
MapTo0 approved these changes Jun 29, 2021
View reviewed changes
Copy link
Member

@MapTo0 MapTo0 left a comment

Choose a reason for hiding this comment

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

lgtm

@hristop hristop merged commit d5ab16a into master Jun 29, 2021
@hristop hristop deleted the ui5-table-group-row branch June 29, 2021 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilhan007 ilhan007 ilhan007 left review comments

@MapTo0 MapTo0 MapTo0 approved these changes

@fifoosid fifoosid fifoosid approved these changes

@elenastoyanovaa elenastoyanovaa Awaiting requested review from elenastoyanovaa

@niyap niyap Awaiting requested review from niyap

@d3xter666 d3xter666 Awaiting requested review from d3xter666

@simonarangelova simonarangelova Awaiting requested review from simonarangelova

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

Successfully merging this pull request may close these issues.

[ui5-table] support grouping by table column content
5 participants
@hristop @MapTo0 @ilhan007 @fifoosid @simonarangelova