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

Wizard: LOCALE - Add Keyboard drop down (HMS-5096) #2651

Merged
merged 3 commits into from
Dec 6, 2024

Conversation

regexowl
Copy link
Collaborator

@regexowl regexowl commented Dec 6, 2024

This adds a drop down for keyboard selection. The options are populated with an output of localectl list-keymaps and sorted in the order of:

  • exact match
  • starting with the search input alphabetically
  • not starting with search input alphabetically

New tests are also added.

image

@regexowl regexowl changed the title Wizard: Add Keyboard drop down (HMS-5096) Wizard: LOCALE - Add Keyboard drop down (HMS-5096) Dec 6, 2024
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 97.91123% with 16 lines in your changes missing coverage. Please review.

Project coverage is 84.80%. Comparing base (60175aa) to head (afed99a).
Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
...izard/steps/Locale/components/KeyboardDropDown.tsx 87.50% 14 Missing ⚠️
src/Utilities/sortfn.ts 91.66% 2 Missing ⚠️

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #2651      +/-   ##
==========================================
+ Coverage   84.26%   84.80%   +0.54%     
==========================================
  Files         170      172       +2     
  Lines       18874    19601     +727     
  Branches     1905     1922      +17     
==========================================
+ Hits        15904    16623     +719     
- Misses       2948     2956       +8     
  Partials       22       22              
Files with missing lines Coverage Δ
...omponents/CreateImageWizard/steps/Locale/index.tsx 100.00% <100.00%> (ø)
...ts/CreateImageWizard/steps/Locale/keyboardsList.ts 100.00% <100.00%> (ø)
...ents/CreateImageWizard/steps/Packages/Packages.tsx 79.63% <100.00%> (-0.42%) ⬇️
src/test/fixtures/blueprints.ts 100.00% <100.00%> (ø)
src/test/fixtures/editMode.ts 100.00% <100.00%> (ø)
src/Utilities/sortfn.ts 91.66% <91.66%> (ø)
...izard/steps/Locale/components/KeyboardDropDown.tsx 87.50% <87.50%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 60175aa...afed99a. Read the comment docs.

@regexowl regexowl force-pushed the locale-keyboard-dropdown branch from 6ea4ac2 to d4fc638 Compare December 6, 2024 10:51
@mgold1234
Copy link
Collaborator

I like it!

mgold1234
mgold1234 previously approved these changes Dec 6, 2024
Copy link
Collaborator

@mgold1234 mgold1234 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@mgold1234
Copy link
Collaborator

mgold1234 commented Dec 6, 2024

small question, if there is no keyboard result, the 'Next' button should become disable?
Screenshot 2024-12-06 at 13 06 22

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [filterValue]);

const sortfn = (a: string, b: string) => {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what do you think about maybe writing all the condtion shorter? something like -

Suggested change
const sortfn = (a: string, b: string) => {
const sortfn = (a: string, b: string) => {
const aKeyboard = a.toLowerCase();
const bKeyboard = b.toLowerCase();
if (aKeyboard === filterValue || aKeyboard < bKeyboard) return -1;
if (bKeyboard === filterValue || bKeyboard < aKeyboard) return 1;
const aStartsWithFilter = aKeyboard.startsWith(filterValue);
const bStartsWithFilter = bKeyboard.startsWith(filterValue);
if (aStartsWithFilter && !bStartsWithFilter) return -1;
if (bStartsWithFilter && !aStartsWithFilter) return 1;
return aKeyboard.localeCompare(bKeyboard);
};

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would this have the same behavior? 🤔

This adds a drop down for keyboard selection. The options are populated with an output of `localectl list-keymaps`.
This adds tests checking functionality of keyboard drop down and blueprint request with locale.
@lucasgarfield
Copy link
Collaborator

I went ahead and rebased on main and resolved the conflicts.

@lucasgarfield
Copy link
Collaborator

small question, if there is no keyboard result, the 'Next' button should become disable? Screenshot 2024-12-06 at 13 06 22

This is an optional customization so we don't need to disable Next.

@lucasgarfield
Copy link
Collaborator

lucasgarfield commented Dec 6, 2024

Noticed a weird bug where if you hit "enter" while using the dropdown and you haven't made a selection it causes the wizard to refresh. This bug affects other fields, too, such as the OpenSCAP step.

#2655

Extracts the function used to sort packages as a utility function so it
can be reused for sorting locales as well.
@lucasgarfield lucasgarfield force-pushed the locale-keyboard-dropdown branch from 5f72555 to afed99a Compare December 6, 2024 22:46
Copy link
Collaborator

@lucasgarfield lucasgarfield left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. Since the bug I noticed (hitting enter while the typeahead is focused can cause a refresh) is also present in other typeaheads, I'm going to say we defer the responsibility to fixing that to another PR that fixes all typeahead behavior.

@lucasgarfield
Copy link
Collaborator

Failing on unit tests in pr_check. Dev checks are passing, so I'm merging.

@lucasgarfield lucasgarfield merged commit 3d75b5b into osbuild:main Dec 6, 2024
6 of 7 checks passed
@regexowl regexowl deleted the locale-keyboard-dropdown branch December 9, 2024 11:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants