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

Fix menu item indentation bug with dynamically slotted icons #2296

Merged
merged 18 commits into from
Jul 30, 2024

Conversation

m-akinc
Copy link
Contributor

@m-akinc m-akinc commented Jul 19, 2024

Pull Request

🀨 Rationale

Fixes #2266

πŸ‘©β€πŸ’» Implementation

Proper indentation relies on startColumnCount being set on the menu items. This is handled by FoundationMenu.setItems. It needs to be called whenever the children of the slotted menu items change. To do this, we need to modify the template so that it watches for any added/removed icons that are children of its menu items (via children directive). I've forked the FAST Menu class (into separate file) and the template to modify them.

These modifications are in menu.foundation.ts rather than index.ts because this is the kind of bug fix that we would have upstreamed to FAST.

πŸ§ͺ Testing

Added unit tests and forked FAST Menu tests.

βœ… Checklist

  • I have updated the project documentation to reflect my changes or determined no changes are needed.

@m-akinc
Copy link
Contributor Author

m-akinc commented Jul 19, 2024

@mollykreis will you please buddy this change?

@m-akinc m-akinc requested a review from mollykreis July 19, 2024 23:32
@m-akinc m-akinc requested a review from mollykreis July 23, 2024 17:19
packages/nimble-components/src/menu/index.ts Outdated Show resolved Hide resolved
@m-akinc m-akinc marked this pull request as ready for review July 23, 2024 18:24
@m-akinc m-akinc requested a review from rajsite July 29, 2024 20:45
@m-akinc m-akinc enabled auto-merge (squash) July 30, 2024 17:40
@m-akinc m-akinc merged commit aadc9dc into main Jul 30, 2024
11 checks passed
@m-akinc m-akinc deleted the users/makinc/fix-dynamic-menu-icon branch July 30, 2024 17:59
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.

menu does not lay out correctly if an item's 'start' slot is dynamically provided
4 participants