Skip to content

Conversation

@camchenry
Copy link
Contributor

@camchenry camchenry commented Jun 27, 2022

@bolonio @hectahertz While integrating with our app, I found a couple of issues that should be addressed.

  1. The internal selected state does not synchronize the selected props that are passed in. So, if the selected state changes asynchronously after the SelectPanel has already started rendering, then the selected state does not appear as expected. Adding an effect to ensure that the internal selection state stays synchronized if the external selections change makes this work as expected.
  2. The click handlers being passed to the close/cancel buttons were passing events as the first argument instead of a gesture string as expected. This caused issues downstream when passing in a custom click handler that always expected a gesture to be passed. Creating two separate handlers, one for the AnchoredOverlay, and one for the close/cancel buttons ensures that we always pass the gesture string correctly.

@camchenry camchenry requested review from a team and rezrah June 27, 2022 16:12
@changeset-bot
Copy link

changeset-bot bot commented Jun 27, 2022

⚠️ No Changeset found

Latest commit: a36cb97

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@bolonio bolonio requested review from bolonio and hectahertz June 28, 2022 15:17
Copy link
Contributor

@hectahertz hectahertz left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thank you for contributing these fixes 🙂.

@hectahertz hectahertz merged commit 0144a21 into primer:primer/issues/1021/reeact-select-panel Jun 28, 2022
Copy link

@bolonio bolonio left a comment

Choose a reason for hiding this comment

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

Thanks for the help :)

@camchenry camchenry deleted the select-panel-fixes branch June 28, 2022 17:34
hectahertz added a commit that referenced this pull request Aug 25, 2022
* Change deprecated/ActionList semantics

* Remove item selection via Enter key

* Handle focus on deprecated/ActionList

* Adapt deprecated/ActionMenu prop

* Update deprecated/ActionList tests and stories

* Update deprecated/ActionList docs

* Remove focusZone from the FilteredActionList component

* Add sr-only description for input in the FilteredActionList component

* Adapt SelectPanel: semantics, visual layout, add title, close button, save/cancel button and functionality. Update tests, docs, and stories.

* Update SelectPanel test snap

* Update deprecated/ActionList axe test

* Update deprecated/ActionList axe test

* Delete commented code

* Update stories, Add ESC reset functionality

* Add changeset (breaking change)

* Preselect items on the SelectPanel docs that aren't the first ones

* Use Heading instead of h1 on SelectPanel

* Don't use selected items as the SelectPanel anchor label

* Replace custom SrOnly with VisuallyHidden

* Make the SelectPanel example dimensions larger

* Support setting the selected items asyncronously

* Fix new select panel issues (#2147)

* Fix onClose being called with event instead of gesture

Co-authored-by: Hector Garcia <[email protected]>

* Remove unsupported PageUpDown bindkeys from useFocusZone

* Remove console.debug

* Make `title` and `inputLabel` optional with defaults

* Remove unused prop

* Add a placeholder prop for the search input

* Make footer buttons smaller

* Use `ButtonClose`

* Better default title for multiselect `SelectPanel`

* Fix lint warnings

* Fix the AutocompleteMenu tests

* Fix lint error

* Fix MarkdownEditor tests, remove one for old behavior

Co-authored-by: Hector Garcia <[email protected]>
Co-authored-by: Cameron McHenry <[email protected]>
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.

3 participants