-
Notifications
You must be signed in to change notification settings - Fork 71
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
Improve keyboard handling and dialog UIs #395
Merged
nrenner
merged 7 commits into
nrenner:master
from
rkflx:pr/improve-keyboard-handling-and-dialog-uis
Apr 2, 2021
Merged
Improve keyboard handling and dialog UIs #395
nrenner
merged 7 commits into
nrenner:master
from
rkflx:pr/improve-keyboard-handling-and-dialog-uis
Apr 2, 2021
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This allows `yarn prettier` to run successfully.
2a43193 made it possible to press `Return` to confirm the export dialog. After 5b52abc this stopped working though, even when the text input had focus. This is due to moving the `<input>` out of the `<form>` and referencing the form's `name` attribute instead. However "The value of [the form] attribute must be the id of a <form> in the same document" according to https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button. Adding such an `id` restores the `Return` key to working properly. The same change is done for the no-go form, where the same issue would otherwise affect a later patch adding similar `Return` key handling. Test Plan: - Create Route, open "Export route" dialog. - Move focus to an input field. - Press `Return`. - The file dialog for saving should open.
5b52abc broke importing no-go areas, in particular after choosing a file and clicking on "Load" nothing would happen. This is due to renaming the `<input>`'s `<id>` which is still referenced in `NogoAreas.js` by its old name. Undoing the rename resolves the issue. Test Plan: - Open "Load no-go areas" dialog. - Click on "Browse" and select GeoJSON file (e.g. as referenced in #161). - Click on "Load". - The no-go areas should be added to the map.
5b52abc copied `<label>` and `<input>` from the `loadedittrackForm` for consistency, but missed to add some corresponding code to `NoGoAreas.js`. After completing the implementation, the filename for loading no-go areas is now displayed after selecting a file just like in the "Load track as route" dialog. Test Plan: - Open "Load no-go areas" dialog. - Browse and select file. - The filename should now be shown next to "Browse".
…s route After ca53080 added Return key handling to the "Delete route" dialog, only the "Load no-go areas" and "Load track as route" dialogs were still missing similar functionality. The implementation is based on 2a43193. Test Plan: - Open "Load no-go areas" or "Load track as route" dialog. - Press Space or Return to open file dialog, choose file, Tab out of file input and press Return to confirm modal. - Notice not having to Tab to the respective confirmation button.
After invoking a shortcut, some dialogs required pressing `Tab` or using the mouse until focus was moved to the primary input field. By moving the focus automatically, users can start typing right away. This is particularly useful in conjunction with the `Return` key for confirming the dialog. Test Plan: - Press `X` to open "Export" dialog: "Name" field has focus. - Press `Shift+O` to open "Load track as route": "Trackfile" is focussed, file dialog opens with `Space`. - Check "Load no-go area" dialog. - Check "POI name" dialog.
Users might want to assign custom tracknames, which requires deleting the default name either entirely or parts of it. By pre-selecting parts of the trackname, users can start typing right away after opening the dialog. `Ctrl+A` to select everything is still possible, but keeping the distance in the filename by default comes in handy, e.g. when using a file manager not displaying the track length. NB: Might need adaptation once trackname validation becomes less strict, i.e. currently "(" and "->" as specified in the message catalog are replaced with ' - ' by the validator before being inserted into the dialog. Test Plan: - Open "Export" dialog ("Location - Other Location - 2km") - Open "Export" dialog for roundtrip ("Location - 1km") - In both cases the complete trackname except for the distance information (including separator) should be selected. - No unwanted behaviour even if Nominatim is slow or down.
nrenner
reviewed
Apr 2, 2021
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.
5b52abc broke importing no-go areas, in particular after choosing a file
and clicking on "Load" nothing would happen.
Oops.
Thanks!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Let's upstream some local changes and fix a couple of regressions in the process, mostly related to keyboard shortcuts and various dialogs:
(See commit messages for more details.)