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

Improve SelectNext props API #1288

Merged
merged 1 commit into from
Oct 4, 2023
Merged

Improve SelectNext props API #1288

merged 1 commit into from
Oct 4, 2023

Conversation

acelaya
Copy link
Contributor

@acelaya acelaya commented Oct 2, 2023

Part of #1285

This PR addresses some of the issues highlighted while using it in a real use-case:

  • Make sure we prevent default behavior of the keyboard event dispatched when pressing ArrowDown on the select toggle, which is used to open the dropdown, and otherwise scrolls the page down a bit.
  • Allow a custom buttonId to be provided, which is passed as the toggle button id if provided. We keep generating a fallback value with useId.
  • Deprecate label and replace it with buttonContent, as label was suggesting a different purpose than the one it is actually used for.

@acelaya acelaya requested a review from robertknight October 2, 2023 16:15
@codecov
Copy link

codecov bot commented Oct 2, 2023

Codecov Report

Merging #1288 (a8cc3d5) into main (386df2a) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1288   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           62        62           
  Lines          900       904    +4     
  Branches       345       348    +3     
=========================================
+ Hits           900       904    +4     
Files Coverage Δ
src/components/input/SelectNext.tsx 100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@robertknight
Copy link
Member

robertknight commented Oct 3, 2023

Extend the props API, allowing all props from HTMLButtonElement to be propagated down to the wrapped Button.

Hmm, I think it is possibly unclear here which element the props will be applied to, since the select control has at least three major elements:

  • An outer container
  • A button
  • A listbox

The general convention in the package is that forwarded props get applied to the outermost element, although I do see at least one exception (Checkbox). A possible solution would be to have specific props to target those elements (eg. buttonProps={...}).

We could also defer this decision by only supporting setting an id prop for the button for the moment.

@acelaya
Copy link
Contributor Author

acelaya commented Oct 3, 2023

Extend the props API, allowing all props from HTMLButtonElement to be propagated down to the wrapped Button.

Hmm, I think it is possibly unclear here which element the props will be applied to, since the select control has at least three major elements:

  • An outer container
  • A button
  • A listbox

The general convention in the package is that forwarded props get applied to the outermost element, although I do see at least one exception (Checkbox). A possible solution would be to have specific props to target those elements (eg. buttonProps={...}).

We could also defer this decision by only supporting setting an id prop for the button for the moment.

If we check the elementRef description, it says Ref associated with component's outermost or primary element.

In most of the components, the outermost and primary elements are the same, but there are exceptions, like the Checkbox you mentioned.

I think SelectNext is another exception. The outermost element is a container div, but the most relevant and primary element is the button.

With this in mind I think it makes more sense (and it is what I would personally expect) that the main props API we expose allows non-specific props to be forwarded to the button.

However, I think it would make sense to have listboxProps={{ ... }} if needed at some point.

I'm also open to start just with the id for now, and not try to solve a problem we don't have yet.

@acelaya acelaya force-pushed the select-next-improvements branch from 6c93459 to 839f9ee Compare October 4, 2023 07:40
@acelaya
Copy link
Contributor Author

acelaya commented Oct 4, 2023

@robertknight in order to move things forward I have reverted the HTMLButtonAttributes change and added a buttonId prop for now.

Let's continue the other discussion when/if needed.

src/components/input/SelectNext.tsx Outdated Show resolved Hide resolved
@acelaya acelaya force-pushed the select-next-improvements branch from 839f9ee to a8cc3d5 Compare October 4, 2023 10:04
@acelaya acelaya merged commit d8064ec into main Oct 4, 2023
@acelaya acelaya deleted the select-next-improvements branch October 4, 2023 10:27
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.

2 participants