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

Enable enter to go to next instructions step #890

Conversation

MarcelRobitaille
Copy link
Collaborator

This is a fix for #882.

When enter is pressed in EditInputGroup, there is an attempt to select the next input field. This works for ingredients, which use inputs, but not for instructions, which use textareas. Now, both are searched for using querySelector.

@seyfeb
Copy link
Collaborator

seyfeb commented Feb 19, 2022

Actually this is not an oversight. What if you wanted to have a line break within your instruction? If we decide to jump to the next line by pressing enter for consistency, we should at least provide a possibility (e.g., shift+enter) to insert line breaks.

@MarcelRobitaille
Copy link
Collaborator Author

Hi @seyfeb. Thanks for reviewing this. I thought of that as well. Maybe we could have one modifier key to insert a newline and another modifier to go to the previous textfield. Or maybe enter alone would be newline, ctrl+enter would be next textfield, and shift+enter would be previous textfield. What do you think?

@seyfeb
Copy link
Collaborator

seyfeb commented Feb 19, 2022

I’m not sure if I have seen such a thing anywhere, but is it a common UI pattern to have "Return/Enter" jump to the previous item?

Or are you suggesting that the (modifier+)enter action should insert a new empty field above the current field?

@MarcelRobitaille
Copy link
Collaborator Author

I was suggesting to jump to the previous field, not create a new one. I guess you are right, I have only seen this with tab / shift+tab.

@seyfeb
Copy link
Collaborator

seyfeb commented Feb 19, 2022

Thinking about this I find the idea to have a possibility to insert a new field above or below the current field with shortcuts also interesting... This should however become a separate issue and be discussed there.

I see two lines of reasoning:

  1. Keep the current behavior and use "Enter" to insert a line break in the current instruction text field because that is how it behaves currently. Use (shift/ctrl/alt)+enter to jump to the next instruction field.

  2. Make the behavior consistent with the ingredients field ("Enter" jumps to next field) and use (shift/ctrl/alt)+enter to create a newline

@christianlupus any thoughts on this?

@christianlupus
Copy link
Collaborator

I would use the same UX as with the tools and ingredients: simple enter switches to next row or spend one. Shift plus enter adds a meeting into the text area.
I think going up is not so critical. For one, there is the tab key. Further, i associate enter with going downward. One could even consider using the arrow keys to navigate over the top of a textbox to the bottom of the box above.

The inserting of intermediate steps is too be discussed in a separate issue in my opinion.

@seyfeb
Copy link
Collaborator

seyfeb commented Feb 20, 2022

Let’s go with that way then: Enter to go to the next item as for ingredients. For inserting a newline I would suggest shift+enter as I have seen this in multiple messengers, so it seems to be a common (and therefore somewhat expected) behavior

@christianlupus christianlupus force-pushed the fix-switch-instruction-enter-882 branch from aa367dc to 8018247 Compare February 24, 2022 17:50
When enter is pressed in EditInputGroup, there is an attempt to select
the next input field. This works for ingredients, which use inputs,
but not for instructions, which use textareas.

Signed-off-by: Marcel Robitaille <marcelrobitaille11@gmail.com>
Signed-off-by: Christian Wolf <github@christianwolf.email>
@christianlupus christianlupus force-pushed the fix-switch-instruction-enter-882 branch from 8018247 to 28cde0f Compare February 24, 2022 18:14
@christianlupus
Copy link
Collaborator

I force-pushed this branch to fix the DCO check. @MarcelRobitaille you need to rebase your local development accordinlgy.

@github-actions
Copy link

github-actions bot commented Feb 24, 2022

Unit Test Results

  22 files    22 suites   9m 32s ⏱️
  66 tests   66 ✔️ 0 💤 0
726 runs  726 ✔️ 0 💤 0

Results for commit b529748.

♻️ This comment has been updated with latest results.

@codecov
Copy link

codecov bot commented Feb 24, 2022

Codecov Report

Merging #890 (b529748) into master (bf5ee39) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #890   +/-   ##
=======================================
  Coverage   21.22%   21.22%           
=======================================
  Files          20       20           
  Lines        1555     1555           
=======================================
  Hits          330      330           
  Misses       1225     1225           
Flag Coverage Δ
integration 7.07% <ø> (ø)
unittests 14.14% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

@christianlupus christianlupus linked an issue Feb 27, 2022 that may be closed by this pull request
Signed-off-by: Christian Wolf <github@christianwolf.email>
@christianlupus christianlupus merged commit 30d0f2d into nextcloud:master Feb 27, 2022
@MarcelRobitaille MarcelRobitaille deleted the fix-switch-instruction-enter-882 branch February 27, 2022 18:06
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.

Switching to next instruction row impossible
3 participants