Skip to content

Feat/full custom select#6865

Merged
PeerRich merged 27 commits intomainfrom
feat/full-custom-select
Feb 13, 2023
Merged

Feat/full custom select#6865
PeerRich merged 27 commits intomainfrom
feat/full-custom-select

Conversation

@sean-brydon
Copy link
Copy Markdown
Member

@sean-brydon sean-brydon commented Feb 3, 2023

What does this PR do?

Code probably needs a bit of cleanup but thought i'd put it out here to see if anyone has any suggestions/improvements first

Full custom select component that matches 95% of WCAG a11y guidelines (I will work on improving in due course - react-select doesnt seem to even be 100%)

This is not used within the app anywhere as of right now. Just been implemented in isolation in storybook.

TODO (maybe in future PR)

  • Where does the focus go after you've select an item when it isn't a multiselect? It doesn't go back to the select itself at least, resulting in that I can not hit space/enter again to reopen the select. I think that's the desired behaviour.

Fixes #6593

Loom Video: https://www.loom.com/share/436b048792514d4b8e6e8907e90bd5b5

Environment: Staging(main branch) / Production

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • Chore (refactoring code, technical debt, workflow improvements)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

@sean-brydon sean-brydon marked this pull request as draft February 3, 2023 11:23
@vercel
Copy link
Copy Markdown

vercel Bot commented Feb 3, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated
cal ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 13, 2023 at 1:52PM (UTC)
ui ✅ Ready (Inspect) Visit Preview 💬 Add your feedback Feb 13, 2023 at 1:52PM (UTC)

Comment on lines +22 to +35
placeHolderRef.addEventListener("keydown", downHandler);
placeHolderRef.addEventListener("keyup", upHandler);
return () => {
placeHolderRef?.removeEventListener("keydown", downHandler);
placeHolderRef?.removeEventListener("keyup", upHandler);
};
} else {
window.addEventListener("keydown", downHandler);
window.addEventListener("keyup", upHandler);
// Remove event listeners on cleanup
return () => {
window.removeEventListener("keydown", downHandler);
window.removeEventListener("keyup", upHandler);
};
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Handle keypress on the red or when in a window

Usefull if you only wanna do stuff when something is focused

Comment on lines +17 to +20
const isSelected =
(Array.isArray(selectedItems) && selectedItems?.some((selection) => selection.value === item.value)) ||
(!Array.isArray(selectedItems) && selectedItems?.value === item.value);

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Check if its selected based if we're using multiple or not

Comment on lines +58 to +65
<div
className={cn(
"flex h-4 w-4 items-center justify-center rounded-[4px] border ltr:mr-2 rtl:ml-2",
isSelected ? "bg-gray-800 text-gray-50" : "text-primary-600 border-gray-300 bg-gray-50"
)}>
{isSelected && <FiCheck className="h-3 w-3 text-current" />}
</div>
</li>
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fake checkbox as the input item would highjack focus and also have weird on event handlers attached

searchBoxRef: React.RefObject<HTMLInputElement>;
}

type FlatternedOption = Option & { current: number; groupedIndex?: number };
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We flattern optional to include the count and also the index of the group it is attached to (If any)

const flatternOptions = (options: Option[], groupCount?: number): FlatternedOption[] => {
return options.reduce((acc, option, current) => {
if (option.options) {
return [...acc, ...flatternOptions(option.options, current + (groupCount || 0))];
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Recursivly call this is we have options -> current + group count keeps track of where we are in the inital list

Needed to reference label and handle keyboard navigation correctly

const upPress = useKeyPress("ArrowUp", searchBoxRef);
const enterPress = useKeyPress("Enter", searchBoxRef);

const flatternedList = useMemo(() => flatternOptions(list), [list]);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

flattern the list on the inital list passed in

Comment on lines +38 to +40
const totalOptionsLength = useMemo(() => {
return flatternedList.length;
}, [flatternedList]);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Calculate the length of this - needed to get the min and max bounds of the array for keyboard navigation

useEffect(() => {
if (downPress) {
// Cycle to start of list if at end
setKeyboardFocus((prev) => (prev + 1) % totalOptionsLength);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

add one to previous take the % of it

e.g list size of 10
10+1 % 10 = 1

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Great use of the modular operator!

useEffect(() => {
if (upPress) {
// Cycle to end of list if at start
setKeyboardFocus((prev) => (prev - 1 + totalOptionsLength) % totalOptionsLength);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Same as above but in reverse

Comment thread packages/ui/components/form/selectimproved/components/Options.tsx
Comment on lines +92 to +103
{filteredList?.map((item, index) => {
const focused = index === keyboardFocus;
return (
<React.Fragment key={index}>
<div className="px-2.5">
{item.current === 0 && item.groupedIndex !== undefined && (
<Label>{list[item.groupedIndex].label}</Label>
)}
<Item item={item} index={index} focused={focused} />
</div>
</React.Fragment>
);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Render all items - if its the first item in the list render a label before it

Comment on lines +26 to +42
classNames?: {
menuButton?: ({ isDisabled }: { isDisabled: boolean }) => string;
menu?: string;
tagItem?: ({ isDisabled }: { isDisabled: boolean }) => string;
tagItemText?: string;
tagItemIconContainer?: string;
tagItemIcon?: string;
list?: string;
listGroupLabel?: string;
listItem?: ({ isSelected }: { isSelected: boolean }) => string;
listDisabledItem?: string;
ChevronIcon?: ({ open }: { open: boolean }) => string;
searchContainer?: string;
searchBox?: string;
searchIcon?: string;
closeIcon?: string;
};
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

WIP styles override if needed

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice — I think this is a super simple approach of overriding the styles. Very easy to use, clear on how it works 👏

Comment on lines +85 to +93
const onPressEnterOrSpace = useCallback(
(e: React.KeyboardEvent<HTMLDivElement>) => {
e.preventDefault();
if ((e.code === "Enter" || e.code === "Space") && !isDisabled) {
toggle();
}
},
[isDisabled, toggle]
);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Handle open on space or enter if its not disabled

Comment thread packages/ui/components/form/selectimproved/components/Select.tsx
Comment thread packages/ui/components/form/selectimproved/components/Options.tsx
@PeerRich
Copy link
Copy Markdown
Member

PeerRich commented Feb 7, 2023

Great new commits @sean-brydon, very happy with what I see. One thing we might be missing still but I think is good if we add straight away is dark mode. Could you check with @Jaibles about this?

not sure, i think the public booking page will continue to use react-select-timezone right?

nevermind. routing forms have selects and darkmdoe

@sean-brydon
Copy link
Copy Markdown
Member Author

Great new commits @sean-brydon, very happy with what I see. One thing we might be missing still but I think is good if we add straight away is dark mode. Could you check with @Jaibles about this?

not sure, i think the public booking page will continue to use react-select-timezone right?

nevermind. routing forms have selects and darkmdoe

Will pick this up tomorrow should be a quick win to implement :)

@sean-brydon
Copy link
Copy Markdown
Member Author

CleanShot 2023-02-08 at 10 08 54@2x

@PeerRich @JeroenReumkens Darkmode been commited :)

@JeroenReumkens
Copy link
Copy Markdown
Contributor

Great work @sean-brydon! Just tested again and noticed two other small issues;

  1. I think the disabled states for section titles got lost when implementing dark mode? I also see that the section title has a checkbox even though you can not check that (or should it work as a "select all below"?)
  2. When selecting options in a multi select, I see the dropdown itself also grows in size. Somehow it feels a bit annoying. What do you think? 🤔

@sean-brydon sean-brydon mentioned this pull request Feb 8, 2023
2 tasks
@PeerRich PeerRich enabled auto-merge (squash) February 8, 2023 16:05
@PeerRich PeerRich disabled auto-merge February 8, 2023 16:05
@PeerRich PeerRich enabled auto-merge (squash) February 8, 2023 16:23
@PeerRich
Copy link
Copy Markdown
Member

PeerRich commented Feb 8, 2023

lemme put this on auto merge, @sean-brydon can you ping @JeroenReumkens once you addressed the changes and @JeroenReumkens can you approve after?

@sean-brydon
Copy link
Copy Markdown
Member Author

@JeroenReumkens I have fixed the disabled state

I can't replicate

When selecting options in a multi select, I see the dropdown itself also grows in size. Somehow it feels a bit annoying. What do you think? 🤔

At all - you got a demo of that happening?

Copy link
Copy Markdown
Contributor

@JeroenReumkens JeroenReumkens left a comment

Choose a reason for hiding this comment

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

Great work @sean-brydon , approved!

@PeerRich PeerRich merged commit c68ff54 into main Feb 13, 2023
@PeerRich PeerRich deleted the feat/full-custom-select branch February 13, 2023 14:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[CAL-846] Improve multiselect component

4 participants