feat(menu): add multi-select & component refactor #133
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's Guide by SourceryThis pull request introduces multi-select functionality to the Class diagram showing the updated AuroMenu component structureclassDiagram
class AuroMenu {
+Boolean multiSelect
+Array~string~ value
+Array~HTMLElement~ optionSelected
+String matchWord
+Boolean noCheckmark
+Boolean loading
+HTMLElement optionActive
+reset()
-handleSelectState(option)
-handleDeselectState(option)
-clearSelection()
-navigateOptions(direction)
-isOptionSelected(option)
-notifySelectionChange()
}
class AuroSelect {
+Boolean multiSelect
+Array~string~ value
+Array~HTMLElement~ optionSelected
+updateDisplayedValue()
}
class AuroCombobox {
+Array~string~ value
+Array~HTMLElement~ optionSelected
-handleInputValueChange()
}
AuroMenu <-- AuroSelect: uses
AuroMenu <-- AuroCombobox: uses
State diagram for menu option selection statesstateDiagram-v2
[*] --> Unselected
Unselected --> SingleSelected: Click (single-select)
Unselected --> MultiSelected: Click (multi-select)
SingleSelected --> Unselected: Click/Reset
MultiSelected --> PartialSelected: Deselect some
PartialSelected --> MultiSelected: Select more
PartialSelected --> Unselected: Deselect all/Reset
MultiSelected --> Unselected: Deselect all/Reset
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
chrisfalaska seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @chrisfalaska - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
62963e0 to
0c01c8b
Compare
There was a problem hiding this comment.
Hey @chrisfalaska - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
jason-capsule42
previously requested changes
Dec 30, 2024
There was a problem hiding this comment.
Refactor selection logic based on our conversation so that the value/optionSelected logic is isolated from the UI updates. And make the UI updates responsive to value/optionSelected in the updated() function rather than triggered directly by the selection event.
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
9baea1f to
1341a73
Compare
chrisfalaska
changed the title
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @chrisfalaska - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider moving event handler bindings from constructor to connectedCallback/disconnectedCallback to prevent potential memory leaks during component lifecycle
- Consider consolidating duplicate event dispatch code into a helper method to improve maintainability and reduce code duplication
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
0c44791 to
b849de0
Compare
sun-mota
requested changes
Jan 7, 2025
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey @chrisfalaska - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 4 issues found
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
sun-mota
reviewed
Jan 8, 2025
jordanjones243
previously requested changes
Jan 8, 2025
I can squash and merge into a single commit. It's one big refactor, I'd argue preserving the commit history is not so important. EDIT: I squashed some of the duplicate-named commits and reduced the overall number by a few. Thank you for the feedback! |
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
84ebf52 to
65b0517
Compare
jordanjones243
force-pushed
the
cfriedberg/123
branch
from
f8e5e18 to
20c646d
Compare
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
5a83ada to
1e30ac2
Compare
sun-mota
requested changes
Jan 16, 2025
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
1e30ac2 to
f3dbb83
Compare
There was a problem hiding this comment.
Hey @chrisfalaska - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a migration guide or breaking changes section in the documentation to highlight the change from string to array values, as this could impact existing implementations.
Here's what I looked at during the review
- 🟡 General issues: 9 issues found
- 🟢 Security: all looks good
- 🟡 Testing: 1 issue found
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
381d421 to
fadfe06
Compare
sun-mota
requested changes
Jan 16, 2025
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
ee7ceba to
301561e
Compare
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
a4e4929 to
dfa6a50
Compare
chrisfalaska
force-pushed
the
cfriedberg/123
branch
from
654f124 to
7f440c6
Compare
sun-mota
approved these changes
Jan 17, 2025
|
🎉 This PR is included in version 2.0.0-beta.11 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Alaska Airlines Pull Request
Multi-select was originally added to
auro-menuas a pending PR in AlaskaAirlines/auro-menu#199.This feature was never merged or integrated but was selected for adoption into
@auro-formkit/auro-menu. This PR incorporates that adoption into theformkitmonorepo.Testing
menu-muli-select.mp4
Summary by Sourcery
Add multi-select functionality to the menu component and implement loading states for combobox, menu, and select components. Update documentation and fix multiple bugs.
New Features:
auro-menu.Tests:
Summary by Sourcery
Add multi-select functionality to the auro-menu component.
New Features:
auro-menu.Tests:
Summary by Sourcery
Add multi-select functionality to the auro-menu component.
New Features:
auro-menu.Tests: