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
Support both selection and text select #515
base: develop
Are you sure you want to change the base?
Conversation
✅ Deploy Preview for lineupjs ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your implementation. I tested this PR with Firefox 94. In my opinion, we should improve the following the points before merging this PR:
Vertical selection
It is unclear when text is selected when dragging vertically (because sometimes no text is selected)? Furthermore, it is unclear why sometimes text is selected in the cell, or the whole row afterwards or even before the starting point? I would expect that no text is selected when I drag a vertical line. Maybe we can work with a time out before highlighting the selected text (in case of a horizontally drag).
Horizontal and diagonal selection
The horizontal selection is also confusing. When I try to select a word (e.g., Europe) it starts selecting the word and at the end jumps to select all the text from the top left to the end of my word. That seems even to happen, while I'm still selecting text in the same cell. I would expect that only the cell is selected as long as I'm not moving to the next cell.
Only the diagonal selection seems to work well and select the text that I would expect from the start to end.
Suggestion
I would imagine that the selection works on a cell level similar to a spreadsheet.
That would have a few advantages:
- The cell provides more space ...
- which allows to detect the direction of the dragging by analyzing the next cell (i.e., above/below = vertical; left/right = horizontal)
- the decission of the direction can be postponed before highlighting the text or selecting the rows
- Text must not be highlighted
- less flickering
- clear communication that values are selected even though they are not visible (e.g., for bar renderer where the values are only visible on selection or hover)
- we might need another color coding for selected cell then
I'm not sure how hard an implemention is or which other functionality would collide with my suggestion.
@sgratzl @alexsb @puehringer What do you think about the demo and my suggestion?
with the option to copy/paste the selected row automatically. How important is this feature anymore? I'm asking cause different browser handle the text selection differently and it works way better what holger was describing in Chrome than in Firefox. |
I see a bit of a problem with full spreadsheet-like selection. What happens if you select a region of elements - a few columns for a few rows. That's something that you can do in a spreadsheet, and it makes sense there because you can edit/format arbitrary groups of cells. To select a whole row you have to click on the row headers. Arbitrary column/row selection isn't so important in lineup, the only use case is copying. Being able to click anywhere in a row to select it seems useful for the LineUp use case. I'd probably be happy if copy for a selected row copies the whole row (you can clean it up in a spreadsheet if you want; or remove the columns you don't want to include), and if I can select text in individual cells. |
@alexsb If I understand you correctly, than this PR should not be merged. Selected rows can already be copied (introduced with PR #510). Copy all rows of a column could for example be solved via an additional menu action |
As far as I can tell, I can't select just a single cell's text in #510. The predominant use case for this PR is the following IMO: I'm looking at a specific item, such as a gene, and want to copy one value, such as it's identifier. Selecting all identifiers of 5 genes would require the spreadsheet like implementation you mention. I think it would be nice, but not sure whether it's really necessary. Instead we could treat this as just a "text select", like it would work if you open up a spreadsheet in a text editor? So, I see three ways forward:
As far as I can tell, this PR is closest to option 2, which includes option 1. I do see a few issues that Holger describes, but I'm not sure we want/need 3. given PR #510. Thoughts? |
the main part that is conflicting with text selection is the drag to select feature. mixing both isn't easy so I would pick one or the other. |
The drag would also conflict with single cell text select (option 1)? I'm not sure how much the drag-select is used (do we have logs?) It's a very cool feature, and I'd not disable it, but possibly limit it to the selection box column. |
what should happen in the taggle overview mode? |
If there's no text visible, than it should not be copyable. Or am I missing any issues here? |
the current fix limits the selection dragging to only when started within the selection column. Thus, you can select text. However, #510 implemented a feature listening to the copy command to copy all selected rows to the clipboard. Not sure how to continue |
closes #505
prerequisites:
Summary
take a look and see how it works. The current logic separates the operation based on the horizontal movement. it has to be less than 30 px to consider a vertical drag