mgr/dashboard: unselect rows in datatables by Sarthak0702 · Pull Request #45313 · ceph/ceph

Sarthak0702 · 2022-03-09T12:18:37Z

User should be able to unselect a selected row in data-tables.
referred from: swimlane/ngx-datatable#775 (comment)

Fixes: https://tracker.ceph.com/issues/53244, https://tracker.ceph.com/issues/54575
Signed-off-by: Sarthak0702 sarthak.0702@gmail.com

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows

nizamial09

LGTM! Thanks @Sarthak0702

nizamial09 · 2022-03-10T10:07:02Z

Is it possible to verify this in a small unit test? @Sarthak0702

pereman2

Great, thanks!

Sarthak0702 · 2022-03-11T15:43:25Z

Is it possible to verify this in a small unit test? @Sarthak0702

Thats a good idea, will try to add that.

Sarthak0702 · 2022-03-14T09:46:30Z

jenkins test windows

Sarthak0702 · 2022-03-16T08:14:18Z

jenkins test make check

Sarthak0702 · 2022-03-16T13:18:02Z

This fix breaks the multi select functionality in data-tables (Cluster-> OSDs ). Need to investigate this.

Sarthak0702 · 2022-03-18T20:27:07Z

This fix breaks the multi select functionality in data-tables (Cluster-> OSDs ). Need to investigate this.

Fixed. As unselect functionality is natively supported by ngx-datatables for 'multiclick' & 'multi', selectCheck is only added for 'single' selectionType.

Sarthak0702 · 2022-03-23T09:45:05Z

jenkins test dashboard

Sarthak0702 · 2022-03-23T09:45:36Z

jenkins test dashboard cephadm

votdev

Nice idea, but i would like to see that this feature behaves the way a desktop user is used to from operating systems like Windows or Gnome, especially Windows Explorer, Nautilus or other file navigation tools. There you can unselect a row by using CTRL + LMB. It is not good to introduce a different workflow instead of implementing a widely used and accepted standard.

Sarthak0702 · 2022-03-23T13:06:02Z

Nice idea, but i would like to see that this feature behaves the way a desktop user is used to from operating systems like Windows or Gnome, especially Windows Explorer, Nautilus or other file navigation tools. There you can unselect a row by using CTRL + LMB. It is not good to introduce a different workflow instead of implementing a widely used and accepted standard.

So are you suggesting to use LMB to select a row but CTRL+LMB to unselect? Wouldn't that be a bit confusing for the user?

votdev · 2022-03-23T13:34:39Z

Nice idea, but i would like to see that this feature behaves the way a desktop user is used to from operating systems like Windows or Gnome, especially Windows Explorer, Nautilus or other file navigation tools. There you can unselect a row by using CTRL + LMB. It is not good to introduce a different workflow instead of implementing a widely used and accepted standard.

So are you suggesting to use LMB to select a row but CTRL+LMB to unselect? Wouldn't that be a bit confusing for the user?

Yes, that is the standard in Windows and Linux UIs.

Sarthak0702 · 2022-03-23T13:57:35Z

Yes, that is the standard in Windows and Linux UIs.

Hmm, I see. FYI, for selectionType:'multiClick' (check Cluster -> OSDs) in ngx-datable (on which TableComponent is built) LMB is used to select and unselect rows natively. So we need to do disable that somehow too.

votdev · 2022-03-23T21:01:45Z

Yes, that is the standard in Windows and Linux UIs.

Hmm, I see. FYI, for selectionType:'multiClick' (check Cluster -> OSDs) in ngx-datable (on which TableComponent is built) LMB is used to select and unselect rows natively. So we need to do disable that somehow too.

Yes i know, this was IMO also not implemented the right way. But it's not my decision which style the Dashboard follows, but i think it should go the same way as the most common operating systems and file navigators do. I mean, the LMB approach to select and unselect will work on touch devices, but i don't think someone is using the Dashboard on those devices.

Sarthak0702 · 2022-03-28T23:50:55Z

Yes i know, this was IMO also not implemented the right way. But it's not my decision which style the Dashboard follows, but i think it should go the same way as the most common operating systems and file navigators do. I mean, the LMB approach to select and unselect will work on touch devices, but i don't think someone is using the Dashboard on those devices.

Honesty I do find using CTRL+ LMB to unselect rows a bit inorganic in a web based application. Have a look at this: https://www.patternfly.org/v4/components/table#composable-row-click-handler-hoverable--selected-rows
Also, surprisingly there are some folks who do use dashboard on touch devices (like iPads). Nevertheless, I and @epuertat would agree, feel that the dashboard tables look a bit outdated, and some modern UI changes would surely improve UX. For this I have opened a tracker issue: https://tracker.ceph.com/issues/55095

epuertat · 2022-04-27T12:10:38Z

jenkins test dashboard

change request to be covered in a different tracker: https://tracker.ceph.com/issues/55095

pereman2 · 2022-05-05T07:43:51Z

jenkins test dashboard

epuertat · 2022-05-05T08:04:32Z

jenkins test dashboard cephadm

pereman2 · 2022-05-05T09:53:43Z

jenkins test dashboard cephadm

pereman2 · 2022-05-05T09:53:52Z

jenkins test dashboard

Sarthak0702 · 2022-05-05T15:58:04Z

jenkins test dashboard

Sarthak0702 · 2022-05-05T21:50:48Z

jenkins test make check

Sarthak0702 · 2022-05-06T05:56:05Z

jenkins test make check

Sarthak0702 · 2022-05-10T19:00:03Z

jenkins test dashboard cephadm

Sarthak0702 · 2022-05-10T19:22:55Z

jenkins test dashboard cephadm

epuertat · 2022-05-13T09:10:45Z

jenkins test api

epuertat · 2022-05-13T09:11:19Z

jenkins test dashboard cephadm

Fixes: https://tracker.ceph.com/issues/53244 Signed-off-by: Sarthak0702 <sarthak.0702@gmail.com>

Sarthak0702 requested a review from a team as a code owner March 9, 2022 12:18

Sarthak0702 requested review from avanthakkar and pereman2 and removed request for a team March 9, 2022 12:18

github-actions bot added dashboard pybind labels Mar 9, 2022

nizamial09 approved these changes Mar 10, 2022

View reviewed changes

pereman2 approved these changes Mar 10, 2022

View reviewed changes

Sarthak0702 added the needs-review label Mar 14, 2022

epuertat added the DNM label Mar 16, 2022

Sarthak0702 force-pushed the unselect-datatable-row branch 2 times, most recently from 1287083 to cfbf9a3 Compare March 18, 2022 20:14

Sarthak0702 force-pushed the unselect-datatable-row branch from cfbf9a3 to 2940734 Compare March 22, 2022 19:10

Sarthak0702 removed the DNM label Mar 22, 2022

Sarthak0702 requested review from nizamial09 and pereman2 March 22, 2022 19:12

votdev previously requested changes Mar 23, 2022

View reviewed changes

Sarthak0702 added the ready-to-merge label Apr 29, 2022

Sarthak0702 force-pushed the unselect-datatable-row branch from 36c424d to 761c112 Compare May 5, 2022 08:01

Sarthak0702 force-pushed the unselect-datatable-row branch from 761c112 to 84d74d8 Compare May 5, 2022 20:12

Sarthak0702 force-pushed the unselect-datatable-row branch 2 times, most recently from 5753c66 to 78ca0cc Compare May 6, 2022 20:51

Sarthak0702 removed the ready-to-merge label May 10, 2022

Sarthak0702 force-pushed the unselect-datatable-row branch 2 times, most recently from d2cbc31 to b69ad53 Compare May 10, 2022 18:24

mgr/dashboard: unselect rows in datatables

b79e2a6

Fixes: https://tracker.ceph.com/issues/53244 Signed-off-by: Sarthak0702 <sarthak.0702@gmail.com>

Sarthak0702 force-pushed the unselect-datatable-row branch from b69ad53 to b79e2a6 Compare May 16, 2022 20:37

Sarthak0702 added ready-to-merge and removed needs-review labels May 17, 2022

epuertat merged commit 5b9dcf3 into ceph:master May 17, 2022

epuertat deleted the unselect-datatable-row branch May 17, 2022 09:48

This was referenced May 18, 2022

pacific: mgr/dashboard: unselect rows in datatables #46322

Merged

quincy: mgr/dashboard: unselect rows in datatables #46323

Merged

nizamial09 mentioned this pull request Oct 10, 2022

mgr/dashboard: refactor dashboard cephadm e2e tests #48163

Merged

14 tasks

Conversation

Sarthak0702 commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

nizamial09 left a comment

Choose a reason for hiding this comment

Uh oh!

nizamial09 commented Mar 10, 2022

Uh oh!

pereman2 left a comment

Choose a reason for hiding this comment

Uh oh!

Sarthak0702 commented Mar 11, 2022

Uh oh!

Sarthak0702 commented Mar 14, 2022

Uh oh!

Sarthak0702 commented Mar 16, 2022

Uh oh!

Sarthak0702 commented Mar 16, 2022

Uh oh!

Sarthak0702 commented Mar 18, 2022

Uh oh!

Sarthak0702 commented Mar 23, 2022

Uh oh!

Sarthak0702 commented Mar 23, 2022

Uh oh!

votdev left a comment

Choose a reason for hiding this comment

Uh oh!

Sarthak0702 commented Mar 23, 2022

Uh oh!

votdev commented Mar 23, 2022

Uh oh!

Sarthak0702 commented Mar 23, 2022

Uh oh!

votdev commented Mar 23, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sarthak0702 commented Mar 28, 2022

Uh oh!

epuertat commented Apr 27, 2022

Uh oh!

pereman2 commented May 5, 2022

Uh oh!

epuertat commented May 5, 2022

Uh oh!

pereman2 commented May 5, 2022

Uh oh!

pereman2 commented May 5, 2022

Uh oh!

Sarthak0702 commented May 5, 2022

Uh oh!

Sarthak0702 commented May 5, 2022

Uh oh!

Sarthak0702 commented May 6, 2022

Uh oh!

Sarthak0702 commented May 10, 2022

Uh oh!

Sarthak0702 commented May 10, 2022

Uh oh!

epuertat commented May 13, 2022

Uh oh!

epuertat commented May 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Sarthak0702 commented Mar 9, 2022 •

edited

Loading

votdev commented Mar 23, 2022 •

edited

Loading