-
Notifications
You must be signed in to change notification settings - Fork 15
chore: add combobox to debug-form and sort cta #884
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
base: main
Are you sure you want to change the base?
Conversation
| padding: 1em; | ||
| box-shadow: 0px 1px 3px rgba(0, 0, 0, 0.2); | ||
| grid-area: header; | ||
| display: flex; |
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.
Minor: Would move out the style to a file at this point.
| state.comboboxInput.addEventListener('keydown', (e) => { | ||
| switch (e.key) { | ||
| case 'ArrowDown': | ||
| e.preventDefault(); | ||
| if (!state.isOpen) { | ||
| openDropdown(); | ||
| } else { | ||
| highlightNext(); | ||
| } | ||
| break; | ||
| case 'ArrowUp': | ||
| e.preventDefault(); | ||
| highlightPrevious(); | ||
| break; | ||
| case 'Enter': | ||
| e.preventDefault(); | ||
| if (state.isOpen && state.highlightedIndex >= 0) { | ||
| selectOption(state.highlightedIndex); | ||
| } | ||
| break; | ||
| case 'Escape': | ||
| e.preventDefault(); | ||
| closeDropdown(); | ||
| break; | ||
| } | ||
| }); |
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.
e.preventDefault(); call can be de-duplicated?
| // Clear existing options | ||
| state.comboboxDropdown?.innerHTML && (state.comboboxDropdown.innerHTML = ''); |
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.
| // Clear existing options | |
| state.comboboxDropdown?.innerHTML && (state.comboboxDropdown.innerHTML = ''); | |
| // Clear existing options | |
| if(state.comboboxDropdown) state.comboboxDropdown.innerHTML = ''); |
More readable and simple expression in this case. Also this is a pattern we use in autofill already!
|
I have a feeling - for something like this (lot of JS code, that we have to understand) - we should just use React or some UI library to reduce the maintanance effort. We might even get some widgets for free, and a much simpler UI. Just food for thought. |
|
I'm not convinced this adds a ton of value and I personally despise comboboxes in place of regular |
Reviewer: @dbajpeyi @GioSensation
Asana: N/D
Description
Update to Debug Form:
Screenshots