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): introduce Single and Multi selection modes #2848

Merged
merged 21 commits into from
Apr 5, 2021

Conversation

niyap
Copy link
Contributor

@niyap niyap commented Feb 19, 2021

Feature: SingleSelect and MultiSelect modes introduced:

  • property mode (SingleSelect | MultiSelect)
  • event selection-change

Usage

<ui5-table mode="SingleSelect"></ui5-table>
<ui5-table mode="MultiSelect"></ui5-table>
table.addEventListener("selection-change",  event => {});

FIXES: #2659

@niyap niyap requested a review from MapTo0 February 19, 2021 17:26
Copy link
Member

@ilhan007 ilhan007 left a comment

Choose a reason for hiding this comment

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

Overall it looks good, besides the comments inline, please check:

  • When rows pop in and the mode is MultiSelect, there is this mixture of white/grey backgrounds, that is not present prior to the change.

Screenshot 2021-02-19 at 8 23 17 PM

  • just to remind to test it in IE


row.selected = !row.selected;

const selectedRows = this.rows.filter(item => item.selected);

if (selectedRows.length === this.rows.length) {
selectAllCheckbox.checked = true;
Copy link
Member

Choose a reason for hiding this comment

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

Would be better if you do this in declarative way, e.g. to create a state (getter or private property) and use in the template:
<ui5-checkbox ?checked={{allRowsSelected}} class="ui5-table-select-all-checkbox">
Can you try if would work this way?

<th class="ui5-table-select-all-column" role="presentation" aria-hidden="true">
<ui5-checkbox class="ui5-table-select-all-checkbox"
?checked="{{_allRowsSelected}}"
@change="{{_selectAll}}"
Copy link
Member

@ilhan007 ilhan007 Mar 31, 2021

Choose a reason for hiding this comment

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

change -> ui5-change (non-conflict event names are recommended)

@@ -191,6 +196,18 @@ const metadata = {
type: Boolean,
},

/**
* Defines the mode of the component (None, SingleSelect, MultiSelect).
Copy link
Member

@ilhan007 ilhan007 Mar 31, 2021

Choose a reason for hiding this comment

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

We will revisit all the API of all the components but this can be improved on the spot, let's use the bullets representation of enum options

        * Defines the mode of the <code>ui5-table</code>.
        * <br><br>
   	 * Available options are:
   	 * <ul>
   	 * <li><code>MultiSelect</code></li>
   	 * <li><code>SingleSelect</code></li>
   	 * <li><code>None</code></li>
   	 * <ul>

},
events: /** @lends sap.ui.webcomponents.main.Table.prototype */ {
/**
* Fired when a row is clicked.
* Fired when a row is clicked or enter key is pressed.
Copy link
Member

@ilhan007 ilhan007 Mar 31, 2021

Choose a reason for hiding this comment

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

enter -> <code>Enter</code>

}

_handleSelect(event) {
const mappings = {
Copy link
Member

Choose a reason for hiding this comment

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

This object will be re-created each time we enter the function

you can do this

this[`_handle${this.mode}`]](event);

or extract the "mappings" as static getter:

static get SELECTION_HANDLERS_MAPPING() {
   return {
            "SingleSelect": "_handleSingleSelect",
			"MultiSelect": "_handleMultiSelect",
    }
}


row.selected = !row.selected;

const selectedRows = this.rows.filter(item => item.selected);
Copy link
Member

Choose a reason for hiding this comment

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

this.rows.filter(item => item.selected) is something you need in several places, add a getter for that:

get selectedRows() {
    return this.rows.filter(item => item.selected)
}

@@ -569,6 +705,20 @@ class Table extends UI5Element {

return this._inViewport ? "absolute" : "sticky";
}

isInViewport() {
Copy link
Member

Choose a reason for hiding this comment

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

Do you use "isInViewport" - it maybe left from merge conflicts resolution, please double check

/**
* @private
*/
firstInRow: {
Copy link
Member

Choose a reason for hiding this comment

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

There is also related code with that, you can remove it :

     // 155:TableRow.js
	cell.firstInRow = (index === 0);

* <b>Note:</b> When set to <code>Active</code>, the item will provide visual response upon press,
* while with type <code>Inactive</code> - will not.
*
* @type {TableRowType}
Copy link
Member

@ilhan007 ilhan007 Mar 31, 2021

Choose a reason for hiding this comment

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

add since especially here, but it won't hurt to add since for the private ones.

For since use 1.0.0-rc.14 (rc.13 has been released)

 * Defines the visual indication and behavior of the <code>ui5-table-row</code>.
 * <br><br>
		 * Available options are:
		 * <ul>
		 * <li><code>Active</code></li>
		 * <li><code>Inactive</code></li>
		 * <ul>
		 ```

*
* @type {boolean}
* @defaultvalue false
* @since 1.0.0-rc.13
Copy link
Member

Choose a reason for hiding this comment

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

13 -> 14

* Fired on selection change of an active row.
*
* @event sap.ui.webcomponents.main.TableRow#selection-requested
* @private
Copy link
Member

Choose a reason for hiding this comment

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

since

}

_onkeydown(event) {
const itemActive = this.type === "Active";
Copy link
Member

Choose a reason for hiding this comment

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

use the type TableRowType.Active instead of hard-coded strings

@@ -46,6 +97,13 @@ const metadata = {
events: /** @lends sap.ui.webcomponents.main.TableRow.prototype */ {
"row-click": {},
Copy link
Member

Choose a reason for hiding this comment

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

Add JS doc when the row-click is fired after this change


static async onDefine() {
await Promise.all([
fetchI18nBundle("@ui5/webcomponents"),
Copy link
Member

Choose a reason for hiding this comment

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

just
await fetchI18nBundle("@ui5/webcomponents"),

use Promise.all for multiple async tasks

@ilhan007
Copy link
Member

ilhan007 commented Apr 1, 2021

FYI: use 1.0.0-rc.15 as since, because we had to release RC.14 due to 2 urgent issues that we fixed and had to release

@ilhan007 ilhan007 merged commit cc31280 into SAP:master Apr 5, 2021
@ilhan007 ilhan007 changed the title feat(ui5-table): SingleSelect and MultiSelect modes introduced feat(ui5-table): introduce Single and Multi selection modes Apr 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support selection modes in ui5-table component
2 participants