Repository navigation
[Remove Vuetify from Studio] Replace CountryField VAutocomplete with KDS KMultiSelect - #6293
LightCreator1007 wants to merge 2 commits into
Conversation
Render KMultiSelect in CountryField instead of the Vuetify VAutocomplete inside DropdownWrapper. The value, required, multiple, label and fullWidth props are unchanged, so the four consumers keep the same English-name values while the list shows translated country names. - Validate required through invalid/invalidText once the field is left - Size the field with appearanceOverrides instead of v-bind() in CSS - Drop the setTimeout that cleared the search text; KMultiSelect does it - Build messages with createMultiSelectMessages and add country strings to commonStrings - Remove Vuetify-only props (box, outline, hide-details, menu-props, clearable) from Create, RequestForm, UserTable and the community library side panel - Rewrite countryField.spec.js with Vue Testing Library - Re-enable the skipped create.spec.js tests and fix the store setup and form filling they needed to pass Closes learningequality#6139
|
👋 Hi @LightCreator1007, thanks for contributing! For the review process to begin, please verify that the following is satisfied:
📢✨ Before we assign a reviewer:
Also check that issue requirements are satisfied & you ran Pull requests that don't follow the guidelines will be closed. Reviewer assignment can take up to 2 weeks. |
🟡 Waiting for changesLast updated: 2026-10-06 03:46 UTC |
rtibblesbot
left a comment
rtibblesbot
left a comment
There was a problem hiding this comment.
PR #6293: CountryField.vue meets the issue's component requirements. I checked it against the KMultiSelect 5.9.0 source. The remaining findings are test assertions.
CI passes. Manual QA did not run, so the UI is not visually verified.
- suggestion: the re-enabled submit test doesn't check that the selected country reaches
register(inline). - suggestion: two
countryField.spec.jsassertions only check that text is present (inline). - nitpick:
countrySelectionsClearedLabelduplicateslanguageSelectionsClearedLabel(inline). - Upstream had two skipped tests, not the four the issue counts. Both now run; no change needed.
- praise: the
create.spec.jsstore now uses theaccountandpoliciesnamespaces thatCreate.vuebinds to. That lets the re-enabled tests reach theregistermock.
Written by rtibblesbot, an LLM-based coding agent.
| await userEvent.click(screen.getByLabelText(/tagging content sources/i)); | ||
|
|
||
| await userEvent.type(screen.getByRole('combobox', { name: /select all that apply/i }), 'Kenya'); | ||
| await userEvent.click(await screen.findByRole('option', { name: 'Kenya' })); |
There was a problem hiding this comment.
suggestion: The submit test doesn't check that the selected country reaches register. It only asserts toHaveBeenCalled() (L144). That passes if locations is empty, holds the translated label, or holds an {id, name} object. Pin the payload:
expect(mockRegisterAction).toHaveBeenCalledWith(
expect.anything(),
expect.objectContaining({ locations: 'Kenya' }),
);This also covers the AC "The value sent to the server stays the English name".
|
|
||
| await leaveField(); | ||
|
|
||
| expect(await screen.findByText('Field is required')).toBeInTheDocument(); |
There was a problem hiding this comment.
suggestion: This assertion doesn't check that the field is marked invalid. It passes whenever the text renders. When invalid is true, KMultiSelect points the combobox's aria-describedby at the error. Assert that wiring:
expect(screen.getByRole('combobox')).toHaveAccessibleDescription('Field is required');|
|
||
| await userEvent.type(screen.getByRole('combobox'), 'zzzzz'); | ||
|
|
||
| expect(await screen.findByText('No countries found')).toBeInTheDocument(); |
There was a problem hiding this comment.
suggestion: This assertion can match "No countries found" outside the dropdown. findByText searches the whole page. Scope it within the listbox, or also assert that no option role remains.
| message: '{count, plural, one {# country selected} other {# countries selected}}', | ||
| context: 'Announced with the number of countries currently selected in a country picker', | ||
| }, | ||
| countrySelectionsClearedLabel: { |
There was a problem hiding this comment.
nitpick: This string duplicates languageSelectionsClearedLabel. It doesn't name the item type, so translators get the same string twice. Replace both with one selectionsClearedLabel shared by the two pickers.
2 participants
Summary
Replaces
VAutocompleteinCountryFieldwith KDSKMultiSelect.value,required,multiple,label,fullWidth), plusdisabled, which the community library panel still uses. Values are still English country names; the list shows translated names.requiredusesinvalid/invalidText. Width usesappearanceOverrides.DropdownWrapper, the::v-deepstyles, the search-clearingsetTimeout, and the Vuetify props on the four pages that use the field.createMultiSelectMessages(as in Replace LanguageDropdown VAutocomplete with KDS KMultiSelect #6154). Two country strings are added tocommonStrings.js.countryField.spec.jswith VTL. Re-enabled the skippedcreate.spec.jstests, which also needed a country and source picked, plus small fixes to the test store.References
Closes #6139.
Reviewer guidance
Screen.Recording.2026-10-06.at.2.05.19.AM.mov
I tested the four flows from the issue in a dev server, with no console errors:
location=Kenyain the URL.pnpm testandpre-commit run --all-filespass.Notes:
AI usage
I used Claude Code to scope the issue against the KMultiSelect docs and source and the earlier migrations. It wrote the implementation and tests, ran Jest and pre-commit, and checked the flows in the browser. I checked the mentioned workflows too, audited the changes to the best fo my abilities.