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

feat(components): Autocomplete Custom Menu Rendering #2229

Merged
merged 48 commits into from
Feb 5, 2025

Conversation

MichaelParadis
Copy link
Contributor

@MichaelParadis MichaelParadis commented Dec 10, 2024

Motivations

As a part of our composability and customizabilty work we wanted to add support for rendering a custom menu in the Autocomplete. To achieve this I added a customMenuRender prop that will be used to replace the content when the Autocomplete menu is opened.

To help achieve this I did the following:

  • Exposed two hooks to aid in handling keyboard navigation with the Autocomplete
    • useKeyboardNavigation for handling the existing keyboard navigation behaviour. This works when the customization of the menu only renders MenuOptions and doesn't include custom elements
    • useCustomKeyboardNavigation this is used when the Autocomplete menu has elements that aren't just options. The reason for this is that the Autocomplete can not be certain of how people want keyboard navigation to work. This works to prevent issues with custom selectable items such as a footer to create another option.
  • Expose various atoms that we use for rendering the content in the Autocomplete menu such as:
    • MenuOption which is the default behaviour of menu options in Autocomplete
    • MenuOptionContent the default rendering of the content of a MenuOption
    • BaseMenuOption the base wrapper of the MenuOption this component handles the highlighted and separator logic
    • Also added UNSAFE_ props for overriding the styles of these atoms

Decisions

I decided to give full access to the Autocomplete Menu including when it is rendered when using the customMenuRender. The reason for this is that the existing Autocomplete wouldn't display a menu if no options were available. However, when custom rendering a Menu the consumer may want to show a "Create new X` option instead.

I made the options type support generics, the reason for this is that if generics weren't provided then the customMenuRender wouldn't be aware of the properties of the options it is rendering which would require consumers to use typescript casting to be able to get the properties of an object for rendering.

I removed the XOR of Option and Group option for OptionCollection the reason for this is that I found it was causing headaches when introducing support for generic option types. I also found that OptionCollection wasn't actually enforcing that initialOptions and getOptions was only an array of Options or GroupOptions

Changes

Added

  • feat(components): Added ability to render a custom menu in the Autocomplete
  • Expose Atoms used for rendering the Menu in Autocomplete

Testing

  • Verify existing Autocomplete stories work as the did previously (verify keyboard navigation and using a mouse, etc.)
  • Verify that the Custom Rendering story can be used with both a keyboard and a mouse
  • Verify existing Autocomplete usages work (see testing branch referencing this PR)

Changes can be
tested via Pre-release


In Atlantis we use Github's built in pull request reviews.

Random photo of Atlantis

Copy link

cloudflare-workers-and-pages bot commented Dec 10, 2024

Deploying atlantis with  Cloudflare Pages  Cloudflare Pages

Latest commit: 3f80505
Status: ✅  Deploy successful!
Preview URL: https://638c0714.atlantis.pages.dev
Branch Preview URL: https://job-111436-rhf-autocomplete.atlantis.pages.dev

View logs

@MichaelParadis MichaelParadis force-pushed the JOB-111436/rhf-autocomplete branch from 5176974 to 30167e5 Compare December 20, 2024 15:43
@MichaelParadis MichaelParadis changed the base branch from master to JOB-110887/add-input-text-rebuild December 20, 2024 16:30
Base automatically changed from JOB-110887/add-input-text-rebuild to master January 14, 2025 18:25
export interface AutocompleteProps<
GenericOption extends AnyOption = AnyOption,
GenericOptionValue extends Option = Option,
GenericGetOptionsValue extends AnyOption = AnyOption,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The generic on GetOptionsValue is because some people were passing an initialOptions value that would be a different type to what was returned by getOptions e.g. Initial options would be a list of GroupOptions but getOptions would give Options based on another value in the form

@@ -28,11 +27,12 @@
}

.options.visible {
display: block;
position: absolute;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the popper visibility to use visibility to match the recommendations in the docs
https://popper.js.org/docs/v2/modifiers/hide/
It also helped with an issue when clicking on customRendered options that didn't use the onMouseDown callback

Copy link

github-actions bot commented Jan 28, 2025

Published Pre-release for aae2a87 with versions:

  - @jobber/[email protected]+aae2a87b

To install the new version(s) for Web run:

npm install @jobber/[email protected]+aae2a87b

@MichaelParadis MichaelParadis changed the title feat(components): Autocomplete WIP Early work feat(components): Autocomplete Custom Menu Rendering Jan 28, 2025
);
}

export const AdvancedTemplateCode = `
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried various ways to get the Canvas to render the code as well, since the Canvas can't use any state and expects a single component to render I had to break it out into AdvancedCustomTemplate but the show code button stopped working.

I tried using things like the source option but it would also not work.

When I tried using the Source block and doing AdvancedCustomTemplate.toString() it was giving minified code. I am fine removing this is we wish

Copy link
Contributor

Choose a reason for hiding this comment

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

I went through basically the exact same process with my stuff, wrestled with it for a while, and wrote this ticket to come back and improve it later. So I think the way you did it is totally fine. I would maybe just suggest using a Disclosure or <details> directly to allow people to show/hide the long blocks of code as they need. Right now it is a bit of a long scroll if you aren't interested in one of the examples

@MichaelParadis MichaelParadis marked this pull request as ready for review January 28, 2025 22:45
@MichaelParadis MichaelParadis requested a review from a team as a code owner January 28, 2025 22:45
@chris-at-jobber
Copy link
Contributor

@MichaelParadis something worth checking in-product - in Storybook, I can have multiple custom Autocompletes open at once:
image

const requestedIndex = options[highlightedIndex + direction];

return requestedIndex && isGroup(requestedIndex)
? direction + direction
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible that direction + direction is also a group?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, the Autocomplete removes GroupOptions that have no other options here. So if requestedIndex is a group it will move to one of the following:

  • The next option which will be inside of the requestedIndex's group
  • The previous option, which will be inside the Group of options "above" in the menu.

There will be a scenario where two group options (with no nested options) next to each other

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay, thank you for clarifying that for me

attachTo: MenuProps["attachTo"],
visible = false,
) {
const [menuRef, setMenuRef] = useState<HTMLElement | null>();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should default to null based on the typing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 59f6974

popper?.update?.();
}, [visible]);

const targetWidth = attachTo.current?.clientWidth;
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also be wrapped in useSafeLayoutEffect in case attachTo is resized after this has already been called?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Popper adjusts a lot of the styling automatically. This targetWidth is helpful for initial positioning but once visible has changed react-popper starts controlling the rendering

@MichaelParadis
Copy link
Contributor Author

@ZakaryH I updated storybook and the docs to include information on rendering Autocompletes with custom sections. I am just updating my pre-release branch right now

@MichaelParadis
Copy link
Contributor Author

@ZakaryH

I've read the comments about keyboard navigation

Exposed two hooks to aid in handling keyboard navigation with the Autocomplete
useKeyboardNavigation for handling the existing keyboard navigation behaviour. This works when the customization of the menu only renders MenuOptions and doesn't include custom elements

useCustomKeyboardNavigation this is used when the Autocomplete menu has elements that aren't just options. The reason for this is that the Autocomplete can not be certain of how people want keyboard navigation to work. This works to prevent issues with custom selectable items such as a footer to create another option.

the first one, is there a reason we can't just provide the keyboard navigation behavior by default? I can't really think of a good argument for not implementing that, and so it makes me wonder why we'd have the consumer build it when using a MenuOption

the second one, yeah I guess that makes sense. it might make more sense once I try some very custom content, but I'm still tempted to provide a baseline keyboard experience

The main reason for this is we could need to call default keyboard behaviour in the parent above and provide the "default" highlighted index as a prop to the renderCustomMenu. I could see 2 possible issues:

  1. Since that prop would update on keyboard events the menu would have extra unneeded rerenders
  2. People might see highlighted index prop and not consider that there may be issues with keyboard navigation. It makes the keyboard navigation a more explicit decision to consider if custom navigation is needed

@ZakaryH
Copy link
Contributor

ZakaryH commented Feb 3, 2025

is there anything different or special about these Add buttons? I noticed they're persisting for just a bit longer than the rest of the menu when I choose an option which is weird since I'd have thought their visibility would be exactly the same as everything else in the menu

Kapture 2025-02-03 at 14 33 50

@MichaelParadis
Copy link
Contributor Author

@ZakaryH I too was confused by this. I pushed an update that also fixes this by adjusting the opacity too 3f80505 The position absolute wasn't needed as well so I cleaned that up.

Ideally I use display: none inside the popper but I found that when this gets toggled the onClick handler on the buttons doesn't work. onMouseDown works but I think it is better to make it "easy" to handle custom element events, especially since other aspects of Custom Menus is already pretty complex and it would add yet another thing to consider

@ZakaryH
Copy link
Contributor

ZakaryH commented Feb 4, 2025

@MichaelParadis nice, yeah that looks much better!

I haven't looked at the code too closely so I'll let @Aiden-Brine give the final approval, but for the things I was playing around with and all the comments I had made this LGTM

Copy link
Contributor

@Aiden-Brine Aiden-Brine left a comment

Choose a reason for hiding this comment

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

Thank you for making those changes and discussing everything! LGTM

@MichaelParadis MichaelParadis merged commit 262a1df into master Feb 5, 2025
13 checks passed
@MichaelParadis MichaelParadis deleted the JOB-111436/rhf-autocomplete branch February 5, 2025 15:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants