Skip to content

✨ <Menu /> support static option for Items #128

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.

Sign up for GitHub

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

Merged
NullVoxPopuli merged 1 commit into from
Jan 17, 2022
Merged

✨ <Menu /> support static option for Items #128

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion addon/components/menu/items.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
{{! template-lint-disable no-down-event-binding }}
{{#if @isOpen}}
{{#if (or @isOpen @static)}}
Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Jan 14, 2022

Choose a reason for hiding this comment

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

do we need @static? I really don't like over-expanding public API, and it seems just setting @isOpen={{true}} would be sufficient?

Copy link
Contributor

@bertdeblock bertdeblock Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It seems the other implementations support a static option as well. So I assume, that's why the PR has been made.

Copy link
Contributor Author

@aoifehannigan aoifehannigan Jan 14, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Yes I included the addition of the static option as it is supported in the React & Vue implementations and is already available in the ListBoxOptions component

In terms of a use case we will be using multiple Menus in a left nav and we need don't require the dropdown to close when focus is lost, only if the user has clicked to close the Menu, or another Menu has been opened. Utilising the static option to override the behavior in our app seemed like the best way to achieve this.

Copy link
Contributor

@far-fetched far-fetched Jan 14, 2022

Choose a reason for hiding this comment

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

Regarding use case:
Somewhere in react doc, I saw information that might be useful for handling with own transition and animation.

Copy link
Contributor Author

@aoifehannigan aoifehannigan Jan 14, 2022

Choose a reason for hiding this comment

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

You're right it's here thank you!

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Jan 17, 2022

Choose a reason for hiding this comment

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

What's the advantage over using isOpen though? I'm confused. Are the semantics really that much better even though the arg does the exact same thing?
Is there a difference in the other implementation?
🤔

Copy link
Contributor

@far-fetched far-fetched Jan 17, 2022

Choose a reason for hiding this comment

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

hmm I think that <Menu.Items/> does not expect to receive isOpen prop. It is in template because is passed down from <Menu/> component, but intention was to not override this prop.
Here

By default, your Menu.Items instance will be shown/hidden automatically based on the internal open state tracked within the Menu component itself.

Probably it is possible to do it without static arg, but then the question is how far we want to be from original API in react and vue

Copy link
Collaborator

@NullVoxPopuli NullVoxPopuli Jan 17, 2022

Choose a reason for hiding this comment

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

ah! so @isOpen is actually private api.

I think perhaps I need to document the public/private ness of all the args 😅

<div
id={{@itemsGuid}}
aria-labelledby={{@buttonGuid}}
Expand Down
15 changes: 15 additions & 0 deletions tests/integration/components/menu-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,6 +219,21 @@ module('Integration | Component | <Menu>', (hooks) => {
assert.dom('[data-test-item-a]').hasTagName('a');
assert.dom('[data-test-item-a]').hasAttribute('href', '/menu');
});

test('should be possible to always render the Menu.Items if we provide it a `static` prop', async function (assert) {
await render(hbs`
<Menu as |menu|>
<menu.Button>Trigger</menu.Button>
<menu.Items data-test-menu-items @static={{true}} as |items|>
<items.Item>Item A</items.Item>
<items.Item>Item B</items.Item>
<items.Item>Item C</items.Item>
</menu.Items>
</Menu>
`);

assert.dom('[data-test-menu-items]').isVisible();
});
});

module('Keyboard interactions', function () {
Expand Down