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

Combobox using Ariakit #218

Open
wants to merge 41 commits into
base: main
Choose a base branch
from
Open

Conversation

ohp-inmeta
Copy link
Contributor

@ohp-inmeta ohp-inmeta commented Feb 3, 2025

Describe your changes

New component MdComboBox, meant to replace MdAutocomplete and MdMultiAutocomplete. Uses Ariakit as base, which handles all necessary aria and accessibility-stuff.

Switch between multi- and single select with value-prop. If an array of strings the combobox is mulitselect, if string; single.

Example of usage in avfallsdeklarering:

Mar-07-2025 20-43-31

Checklist before requesting a review

  • I have performed a self-review and test of my code
  • I have added label to the PR (major, minor or patch)
  • If new component: Is story for component created in stories-folder?
  • If new component: Is tsx-file import added to packages/react/index.tsx?
  • If new component: Is css-file added to packages/css/index.css?

@ohp-inmeta ohp-inmeta requested a review from a team as a code owner February 3, 2025 08:09
Copy link
Contributor

github-actions bot commented Feb 3, 2025

Please set a versioning label of either major, minor, or patch to the pull request.

@ohp-inmeta ohp-inmeta linked an issue Feb 3, 2025 that may be closed by this pull request
@aurorascharff
Copy link
Contributor

aurorascharff commented Feb 5, 2025

Oppdaget at når man velger et element i lista med enter, så mister man fokus på lista og må "begynne på nytt".

@aurorascharff

This comment was marked as resolved.

@aurorascharff
Copy link
Contributor

Legger inn eksemplet Ola sendte til meg som hans referanse: https://ariakit.org/examples/combobox-multiple
Så kan man teste litt mot det.

Fixed issue where focus shifted to input field after selecting item with keyboard. Possibly also fixed weird focus border on zoomed out screens?
@ohp-inmeta
Copy link
Contributor Author

Oppdaget at når man velger et element i lista med enter, så mister man fokus på lista og må "begynne på nytt".

Fikset

@ohp-inmeta
Copy link
Contributor Author

Oppdaget at ved 90% zoom virke det som border ikke er 2px men 1px, og derfor blir padding feil når man fokuserer. I tillegg blir plasseringen i høyden til placeholder ikke alignet riktig.

Trolig også fikset, klarer ikke gjenskape lenger. Mulig dette løste seg når jeg fiksa den focus-saken? 🤔

@aurorascharff

This comment was marked as resolved.

@ohp-inmeta

This comment was marked as resolved.

@ohp-inmeta

This comment was marked as resolved.

@aurorascharff
Copy link
Contributor

aurorascharff commented Feb 11, 2025

La merge til at dropdown-animasjonen ikke lenger er med. Men den er kanskje mest irriterende uansett?

La på frekk animasjon ved åpne/lukke 🥂

Daaaamnnn den var smooth! Komponenten er jo helt rå nå!

@ohp-inmeta
Copy link
Contributor Author

ohp-inmeta commented Feb 11, 2025

La på en console.warn om deprecation på MdAutocomplete - vet ikke om det er lurt eller ikke?

@aurorascharff
Copy link
Contributor

Får en litt rar oppførsel når dropdownen ikke får plass. Ser ut som den prøver å justere plasseringen sin, kanskje det burde skrus av?
Mar-04-2025 14-26-37
](url)

@ohp-inmeta

This comment was marked as resolved.

ohp-inmeta and others added 2 commits March 5, 2025 18:57
fix no results styling
added readme for combobox
@aurorascharff

This comment was marked as resolved.

defaultOptions?: MdComboBoxOption[];
value: string | string[];
disabled?: boolean;
errorText?: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Vi bruker ofte error + errorText, det burde kanskje også være her? Eller er det best å gå bort i fra det?

const [helpOpen, setHelpOpen] = useState(false);

useEffect(() => {
onSelectOption(selectedValues);
Copy link
Contributor

@aurorascharff aurorascharff Mar 8, 2025

Choose a reason for hiding this comment

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

denne trigger når komponenten rendres. Det ødelegger for eksempel for skjemavalidering her:

                onSelectOption={value => {
                  console.log('value', value);
                  form.setFieldValue('ealHovedkapittel', value);

Siden setFieldValue har blitt touched pga komponenten trigget onSelectOption, får jeg "required" feilmelding før brukeren valgte noe.

Tror det er litt useeffect antipattern uansett, bedre at vi setter staten i eventen som trigger endringer i state:

     <Ariakit.ComboboxProvider
      id={comboBoxId}
      selectedValue={selectedValues}
      setSelectedValue={values => {
        setSelectedValues(values);
        onSelectOption(values);
      }}        

Jeg pusher den endringen nå for å teste videre! :)

@aurorascharff
Copy link
Contributor

aurorascharff commented Mar 8, 2025

Har fremdeles litt trøbbel med at onSelect trigger en FPS drop og at ComboBoxItem rerendrer veldig mange ganger. Vi bør sjekke ut dette litt mer. Jeg debugger:

Sykt mange rerenders:
image

Ariakit har insane mange providers:
image

Det er ikke såå ille, men litt rart.
kan være det er noe galt i mitt prosjekt.

return defaultOptions;
}

// return matchSorter(options, searchValue, { keys: ['value'], threshold: matchSorter.rankings.CONTAINS });
Copy link
Contributor

Choose a reason for hiding this comment

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

Denne linja bør kanskje fjernes?

@aurorascharff
Copy link
Contributor

aurorascharff commented Mar 8, 2025

La merge til at eslint-plugin ikke funker riktig for react-hooks, la det inn i denne branchen og fant dette:
image
Jeg fikser det nå!

const [searchValue, setSearchValue] = useState('');
const [selectedValues, setSelectedValues] = useState<string[] | string>(value);
const [helpOpen, setHelpOpen] = useState(false);

Copy link
Contributor

@aurorascharff aurorascharff Mar 8, 2025

Choose a reason for hiding this comment

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

Fant ut at man må håndtere caset der values endres fra utenfor komponenten, eks i mitt tilfelle for en filtrering som har en "reset all filters"-knapp som resetter verdien i Combobox. Combobox sine selectedValues er ikke da lenger i sync med values.

Legger på en usEffect for å fikse det:
useEffect(() => {
setSelectedValues(value);
}, [value]);

Men. Hva skal vi gjøre med searchValue? Er det greit at den henger igjen? Da ser det litt ut som man har valgt noe.

Kan eventuelt da endre til dette:
useEffect(() => {
setSelectedValues(value);
if (!value) {
setSearchValue('');
}
}, [value]);

Det funker ganske bra:
Mar-08-2025 22-31-12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite dropdown components to be accessible and free of bugs
4 participants