Skip to content

Space key on MultiSelectionRenderer combobox selects grid row#10388

Merged
tsuoanttila merged 2 commits into
vaadin:masterfrom
mcollovati:ISSUE-10221/select-row-with-space
Jan 9, 2018
Merged

Space key on MultiSelectionRenderer combobox selects grid row#10388
tsuoanttila merged 2 commits into
vaadin:masterfrom
mcollovati:ISSUE-10221/select-row-with-space

Conversation

@mcollovati

@mcollovati mcollovati commented Nov 28, 2017

Copy link
Copy Markdown
Contributor

Adds a key up handler on MultiSelectionRenderer checkbox
in order to allow row selection pressing space key

Fixes #10221


This change is Reviewable

@tsuoanttila

Copy link
Copy Markdown
Contributor

Suggesting to re-use existing test UI for this. If you're busy and can't migrate the test, we can go with this.


Reviewed 3 of 3 files at r1.
Review status: all files reviewed at latest revision, 2 unresolved discussions.


uitest/src/main/java/com/vaadin/tests/components/grid/GridMultiselectSelectWithSpaceKey.java, line 7 at r1 (raw file):

import com.vaadin.ui.Grid;

public class GridMultiselectSelectWithSpaceKey extends AbstractTestUI {

Instead of making a whole new test class for this, you could extend the existing tests in GridSelectionTest. It already contains helper methods for testing selection.


uitest/src/test/java/com/vaadin/tests/components/grid/GridMultiselectSelectWithSpaceKeyTest.java, line 42 at r1 (raw file):

        for (int row : rows) {
            boolean isRowSelected = grid.getRow(row).isSelected();
            if (selected) {

This whole if block could be replaced by assertEquals("<enter good message here>", selected, grid.getRow(row).isSelected());

I guess the good message is a bit tricky to come up with though.. Can live without the change.


Comments from Reviewable

@mcollovati

Copy link
Copy Markdown
Contributor Author

I can migrate the test during this weekend but feel free to do it on your own if you think it's better


Comments from Reviewable

@mcollovati

Copy link
Copy Markdown
Contributor Author

uitest/src/main/java/com/vaadin/tests/components/grid/GridMultiselectSelectWithSpaceKey.java, line 7 at r1 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…

Instead of making a whole new test class for this, you could extend the existing tests in GridSelectionTest. It already contains helper methods for testing selection.

Done.


Comments from Reviewable

@mcollovati

Copy link
Copy Markdown
Contributor Author

uitest/src/test/java/com/vaadin/tests/components/grid/GridMultiselectSelectWithSpaceKeyTest.java, line 42 at r1 (raw file):

Previously, tsuoanttila (Teemu Suo-Anttila) wrote…

This whole if block could be replaced by assertEquals("<enter good message here>", selected, grid.getRow(row).isSelected());

I guess the good message is a bit tricky to come up with though.. Can live without the change.

Replaced with existing assertion present in GridSelectionTest


Comments from Reviewable

@mcollovati

Copy link
Copy Markdown
Contributor Author

uitest/src/main/java/com/vaadin/tests/components/grid/GridMultiselectSelectWithSpaceKey.java, line 7 at r1 (raw file):

Previously, mcollovati (Marco Collovati) wrote…

Done.

Moved test into GridSelectionTest


Comments from Reviewable

@knoobie

knoobie commented Dec 11, 2017

Copy link
Copy Markdown
Contributor

Would it be possible to see this in 8.2 as well? The grid related patchnotes and this complement one another really well.

Adds a key up handler on MultiSelectionRenderer checkbox
in order to allow row selection pressing space key

Fixes vaadin#10221
@mcollovati mcollovati force-pushed the ISSUE-10221/select-row-with-space branch from be15f98 to b9f490f Compare December 27, 2017 15:35
@tsuoanttila

Copy link
Copy Markdown
Contributor

Reviewed 3 of 3 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

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.

3 participants