-
Notifications
You must be signed in to change notification settings - Fork 12
Make license field easier to use in browsers #69
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
Conversation
Chrome, Firefox, and Safari have small differences when it comes to behaviour of datalist. This PR makes the behaviour more predictable and more convenient for a user to select licenses: - Can click on list and immediately confirm the selection of the license (without having to hit Enter again, as it normally be on Firefox) - Can type license ID, in any casing, and hit Tab/Enter one time to confirm the selection of the license Tested with Chrome, Firefox, and Safari License field now listens to three events (Enter/Tab only available to check in keystroke events such as 'keydown'). See code comments for implementation details. Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
|
I don't think consistency across browsers is a goal in itself (because then the app is inconsistent with the rest of the user's browser/OS, which is what matters the most). But this PR definitely makes the license selector more usable, so that's good. Could you add Cypress tests? |
|
Thank you, I have never used Cypress before but will try. |
There's still a different behaviour of typing to be debug Signed-off-by: Arthit Suriyawongkul <[email protected]>
Signed-off-by: Arthit Suriyawongkul <[email protected]>
|
Cypress tests added. Cover duplication test, casing test, mouse click (simulated) test, and confirmation test ("MIT-0" vs "MIT") test. |
'confirmed' will always be true at line 77, remove it
js/fields_data.js
Outdated
| // Chrome: Detach/re-attach the datalist to hide the popup after insertion. | ||
| var ua = (navigator.userAgent || ''); | ||
| var isChrome = /Chrome/.test(ua) && !/Edg|OPR|Brave|CriOS/.test(ua); | ||
| if (isChrome) { | ||
| licenseField.removeAttribute('list'); | ||
| setTimeout(() => { | ||
| licenseField.setAttribute('list', 'licenses'); | ||
| }, 0); | ||
| } |
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.
what happens if we do this unconditionally?
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.
Can you explain more about it please? What condition? Around which lines of code please? Thank you
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.
If you mean !isChrome, non-Chrome browsers don't have this issue. The popup immediately disappear after insertion.
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.
I did mean isChrome. If we remove it, it will automatically work for all Chromium derivatives not explicitly listed here (plus WebKit browsers like Safari?), and there is no harm in removing for non-Chromium browsers
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.
Oddly, on Safari, the reattachment of the datalist will force the list to popup again :(
I will put this in the code comment.
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.
🤦
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.
Updated the code and the comment after manually tested it additionally with Chromium, Brave, and Vivaldi.
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!
Tested manually with Vivaldi and Brave
|
Is this good to merge? |
|
It's good now :) |
|
thanks! |
Chrome, Firefox, and Safari have small differences when it comes to behaviour of datalist. This PR makes the behaviour more predictable and more convenient for a user to select licenses:
Tested with Chrome, Firefox, and Safari.
Demo: https://bact.github.io/codemeta-generator/
Implementation
input,change(existing), andkeydownkeydown