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 aside menu #827

Merged
merged 5 commits into from
Sep 12, 2022
Merged

Fix aside menu #827

merged 5 commits into from
Sep 12, 2022

Conversation

KamiKillertO
Copy link
Contributor

There's currently an issue with the aside menu button in the live demo app.
image

When we click on it, we are redirected to the documentation home page.
With this fix the aside menu button will now works as expected, when clicking on it it will open the side menu
image

I've also added a focus trap when the menu is opened

@lukemelia
Copy link
Collaborator

Hi @KamiKillertO Thanks for working on this. It looks like linting is failing on this PR. Can you fix that?

@KamiKillertO
Copy link
Contributor Author

Sorry for that 😅

@KamiKillertO
Copy link
Contributor Author

@lukemelia Done 👍

package.json Outdated Show resolved Hide resolved
@bertdeblock
Copy link
Collaborator

Seems there's still a conflict. Probably because I merged #836, sorry for that. Looks good to go otherwise!

@lukemelia lukemelia merged commit 69f84c4 into chrislopresto:master Sep 12, 2022
@lukemelia lukemelia added the bug label Sep 14, 2022
@bertdeblock
Copy link
Collaborator

We're running into embroider-build/embroider#1175 because of the ember-focus-trap addition. This isn't an issue with ember-freestyle itself, but just wanted to point out that this change might break people's Embroider based builds.

@lukemelia
Copy link
Collaborator

@bertdeblock What is the ideal resolution from your perspective?

@bertdeblock
Copy link
Collaborator

Other than fixing embroider-build/embroider#1175, I'm not sure to be honest.
The current workaround seems to be for the host app to install ember-focus-trap as well.
We could create an issue in ember-freestyle explaining this or even have this documented in the README or something until the bug is fixed. The only downside I guess is, we recommend to exclude ember-freestyle for production builds. If the host app itself doesn't use ember-focus-trap they end up with unused code in production builds unless they exclude ember-focus-trap manually themselves.

@bertdeblock
Copy link
Collaborator

FYI, embroider-build/embroider#1278 should fix this issue for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants