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

bugfix: first multiselection has no _lastSelectedIndex #788

Merged
merged 1 commit into from
Nov 22, 2019

Conversation

frykten
Copy link
Contributor

@frykten frykten commented Nov 20, 2019

#787

Should I add a test?

@mixonic
Copy link
Member

mixonic commented Nov 20, 2019

@frykten Yes this should certainly get a test. I'm a bit surprised the functionality isn't already covered by a test. It is because the tested functionality uses click, then shift+click? And your issue is trigged by shift+click without a preceding click?

@frykten
Copy link
Contributor Author

frykten commented Nov 20, 2019

@mixonic Yes, that's exactly it.

What would you advise: change the method selectRange() used to select from a normal click first => shift+click second to shift+click first => shift+click second or since both should be available, let's test both?

@mixonic
Copy link
Member

mixonic commented Nov 20, 2019

Ah yes, that is the case:

async selectRange(beginIndex, endIndex) {
await this.body.rows.objectAt(beginIndex).click();
await this.body.rows.objectAt(endIndex).clickWith({ shiftKey: true });
},
the test always uses a click as the starting input, then the second click is shift+click.

I think that means we have essentially left undefined what the behavior for shift+click when there is nothing selected should be.

Some existing UX to consider:

  • In the MacOS finder shift+click when nothing is selected actually selects from item Initial cleanup of gh-pages #1 to the clicked item.
  • In a <select multiple> shift+click with nothing selected simply acts like a click.
  • In a browser selecting text on a webpage shift+click with nothing selected acts like a click.
  • In Google Sheets shift+click with nothing selected effectively select from the first item (the top-left cell) to the cell clicked.

Are there other cases to consider? We just need to make a design decision, I will raise it with some other maintainers as well.

@mixonic
Copy link
Member

mixonic commented Nov 20, 2019

IMO selectRange should stay as-is, and we should add tests for this specific case.

@frykten
Copy link
Contributor Author

frykten commented Nov 21, 2019

@mixonic I don't see any other cases.
Could be interesting to chat about it, right. Gotta admit, we're more into shift+click with nothing selected simply acts like a click, here, but the other design is a good idea also.

I fixed-up the PR to add the test.

@mixonic
Copy link
Member

mixonic commented Nov 21, 2019

@frykten ok today I got to sync up with @bantic and @twokul and we think having a shift+click without any existing selection work as a click is fine. I think this PR already reflects that 👍

@bantic looked at the CI quickly and it seems a point revision of prettier is causing the build to fail :-/

@mixonic
Copy link
Member

mixonic commented Nov 22, 2019

CI on master fixed as of #790, @frykten can you rebase this?

@frykten
Copy link
Contributor Author

frykten commented Nov 22, 2019

Done @mixonic

@mixonic mixonic merged commit 61a9cd0 into Addepar:master Nov 22, 2019
@mixonic
Copy link
Member

mixonic commented Nov 22, 2019

Great! Thank you @frykten I'm not sure we will make a release for a week or two because of the holiday, but keep an eye out.

@frykten frykten deleted the fix_selection branch November 26, 2019 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants