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

Support both selection and text select #515

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from

Conversation

sgratzl
Copy link
Member

@sgratzl sgratzl commented Nov 24, 2021

closes #505

prerequisites:

  • branch is up-to-date with the branch to be merged with, i.e. develop
  • build is successful
  • code is cleaned up and formatted

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

@sgratzl sgratzl added the type: feature New feature or request label Nov 24, 2021
@sgratzl sgratzl requested a review from thinkh November 24, 2021 21:39
@sgratzl sgratzl self-assigned this Nov 24, 2021
@netlify
Copy link

netlify bot commented Nov 24, 2021

Deploy Preview for lineupjs ready!

Name Link
🔨 Latest commit 73ffb16
🔍 Latest deploy log https://app.netlify.com/sites/lineupjs/deploys/62f2f8c9087fd40008e1bb6d
😎 Deploy Preview https://deploy-preview-515--lineupjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

Copy link
Member

@thinkh thinkh left a 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).

lineup-vertical-selection

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.

lineup-horizontal-selection

Suggestion

I would imagine that the selection works on a cell level similar to a spreadsheet.

grafik

lineup-selection-text

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?

@sgratzl
Copy link
Member Author

sgratzl commented Jan 12, 2022

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.

@alexsb
Copy link
Contributor

alexsb commented Jan 13, 2022

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.

@thinkh
Copy link
Member

thinkh commented Jul 29, 2022

@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 Copy content. (This functionality should be implemented in a different PR.) Can we then close the related issue #505 or is there a different solution to it?

@alexsb
Copy link
Contributor

alexsb commented Aug 2, 2022

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:

  1. Single cell text select: you only get to copy text from an individual selected cell
  2. Text-editor like text select: you copy what you highlighted
  3. Spreadsheet like text select.

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?

@sgratzl
Copy link
Member Author

sgratzl commented Aug 2, 2022

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.

@alexsb
Copy link
Contributor

alexsb commented Aug 2, 2022

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.

@sgratzl
Copy link
Member Author

sgratzl commented Aug 5, 2022

what should happen in the taggle overview mode?

@alexsb
Copy link
Contributor

alexsb commented Aug 5, 2022

If there's no text visible, than it should not be copyable.
For selected rows, where text is visible, the behaviour should be the same.

Or am I missing any issues here?

@sgratzl
Copy link
Member Author

sgratzl commented Aug 10, 2022

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

@sgratzl sgratzl added the status: help wanted Extra attention is needed label Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: help wanted Extra attention is needed type: feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants