Skip to content

Fix behavior of multiselect in list view#3161

Merged
yadvr merged 1 commit into
apache:masterfrom
pbankonier:bugfix_multiselect_behavior_listview
Mar 26, 2019
Merged

Fix behavior of multiselect in list view#3161
yadvr merged 1 commit into
apache:masterfrom
pbankonier:bugfix_multiselect_behavior_listview

Conversation

@pbankonier

@pbankonier pbankonier commented Feb 1, 2019

Copy link
Copy Markdown
Contributor

Description

How to reproduce:

  1. Select one entry in a multiSelect listView (e.g. instance list) and then deselect it again. Now click on the "select all" checkbox. The entries which were clicked previously are not displayed as selected.

  2. Select at least one entry. Use the multiSelectAction buttons to trigger an action. Wait for completion. The list refreshes and no entry is selected but the multiSelectAction buttons are displayed.

Fix:

  1. Use "checked" property instead of the attribute to select all entries. This is necessary because when you checked an entry by clicking on it the property gets set and the browser is preferably using this. So setting the attribute by clicking on the "select all" checkbox had no impact anymore on the checkBox of the previously clicked entries.

  2. Hide the multiSelectAction buttons when the list is refreshed after an action is performed and no element is selected anymore.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)

How Has This Been Tested?

Deployed to our test environment and tested functionality.

Philipp Bankonier
Fix behavior of multiselect in list view
89c1d28 
Hide multiselectaction buttons when selection is resetted after a action is performed
Use checked prop instead of attr to show/hide triggered selection when box was clicked by a user before
@svenvogel

svenvogel commented Feb 1, 2019

Copy link
Copy Markdown
Contributor

@pbankonier nice!

@svenvogel

svenvogel commented Feb 1, 2019

Copy link
Copy Markdown
Contributor

@rhtyd @rafaelweingartner @GabrielBrascher can take a look :)

@yadvr

yadvr commented Feb 1, 2019

Copy link
Copy Markdown
Member

Couldn't reproduce this with 4.11.2, likely an issue on master due to jquery version change.

@pbankonier

pbankonier commented Feb 1, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Couldn't reproduce this with 4.11.2, likely an issue on master due to jquery version change.

Yes it is related to the jquery version update.
https://jquery.com/upgrade-guide/3.0/#breaking-change-removeattr-no-longer-sets-properties-to-false
I also can only reproduce it on master. In 4.11.2 it worked.

GabrielBrascher
GabrielBrascher approved these changes Feb 8, 2019
View reviewed changes

@GabrielBrascher GabrielBrascher left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Code LGTM, additionally I tested and it works.
Thanks @pbankonier

borisstoyanov
borisstoyanov approved these changes Feb 11, 2019
View reviewed changes

@borisstoyanov borisstoyanov left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, no need for marvin tests since it's in the UI only

@svenvogel

svenvogel commented Feb 11, 2019

Copy link
Copy Markdown
Contributor

@rhtyd can we merge it? can you have a look? 👍

@GabrielBrascher

GabrielBrascher commented Feb 11, 2019

Copy link
Copy Markdown
Member

@svenvogel The master branch is in freeze state due to the 4.12 releasing process. As soon as we have master open for merging we can merge this one. Thanks!

@svenvogel

svenvogel commented Feb 12, 2019

Copy link
Copy Markdown
Contributor

@GabrielBrascher okay i see! thanks for the info.

@yadvr yadvr added this to the 4.13.0.0 milestone Mar 26, 2019
@yadvr
yadvr merged commit 2e2b670 into apache:master Mar 26, 2019
pbankonier added a commit to pbankonier/cloudstack that referenced this pull request Apr 24, 2019
@pbankonier Philipp Bankonier
ui: Fix behavior of multiselect in list view (apache#3161)
6aead14 
Hide multiselectaction buttons when selection is resetted after a action is performed
Use checked prop instead of attr to show/hide triggered selection when box was clicked by a user before.

How to reproduce:

Select one entry in a multiSelect listView (e.g. instance list) and then deselect it again. Now click on the "select all" checkbox. The entries which were clicked previously are not displayed as selected.

Select at least one entry. Use the multiSelectAction buttons to trigger an action. Wait for completion. The list refreshes and no entry is selected but the multiSelectAction buttons are displayed.

Fix:

Use "checked" property instead of the attribute to select all entries. This is necessary because when you checked an entry by clicking on it the property gets set and the browser is preferably using this. So setting the attribute by clicking on the "select all" checkbox had no impact anymore on the checkBox of the previously clicked entries.

Hide the multiSelectAction buttons when the list is refreshed after an action is performed and no element is selected anymore.
@weizhouapache weizhouapache mentioned this pull request Jan 23, 2020
Closed
@DaanHoogland

DaanHoogland commented Jan 24, 2020

Copy link
Copy Markdown
Contributor

@pbankonier please see @weizhouapache 's remarks in the linked issue above #3827. Can you have a look?
cc @svenvogel @GabrielBrascher

@ustcweizhou ustcweizhou mentioned this pull request Jan 24, 2020
Merged
5 tasks
yadvr pushed a commit that referenced this pull request Jan 31, 2020
@ustcweizhou
ui: fix cannot assign vms to internal lb in VPC (#3832)
75aa6e3 
This fixes the issue that cannot assign VMs in Internal LB in VPC.
listView is overwritten due to commit 2e2b670 (#3161)

Fixes: #3827
ustcweizhou added a commit to ustcweizhou/cloudstack that referenced this pull request Feb 28, 2020
@ustcweizhou
ui: fix cannot assign vms to internal lb in VPC (apache#3832)
3e9da2d 
This fixes the issue that cannot assign VMs in Internal LB in VPC.
listView is overwritten due to commit 2e2b670 (apache#3161)

Fixes: apache#3827
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@GabrielBrascher GabrielBrascher GabrielBrascher approved these changes
@svenvogel svenvogel svenvogel approved these changes
@borisstoyanov borisstoyanov borisstoyanov approved these changes

Assignees

@pbankonier pbankonier

Labels

component:UI type:bug

Projects

None yet

Milestone

4.13.0.0

Development

Successfully merging this pull request may close these issues.

6 participants

@pbankonier @svenvogel @yadvr @GabrielBrascher @DaanHoogland @borisstoyanov