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

Move control of sidebar width to its menu child #5575

Merged
merged 1 commit into from
Nov 23, 2020

Conversation

djhi
Copy link
Collaborator

@djhi djhi commented Nov 23, 2020

No description provided.

@djhi djhi added the RFR Ready For Review label Nov 23, 2020
@djhi djhi added this to the 3.10.3 milestone Nov 23, 2020
@fzaninotto fzaninotto merged commit 6caea4b into master Nov 23, 2020
@fzaninotto fzaninotto deleted the sidebar-menu-width-control branch November 23, 2020 16:52
@fzaninotto
Copy link
Member

@djhi This PR doesn't have a description or a use case. The changelog says "Fix <Sidebar> width cannot be modified by the child <Menu>". Do you remember why we did this?

@fzaninotto
Copy link
Member

I think that's because we want to allow menus of different width (like an icon menu), so it's the menu's responsibility to define its width, not its container's.

@fzaninotto
Copy link
Member

Found it. We did this because ra-enterprise removes the width handling from the Sidebar to allow it to be handled by the child Menu. But when a developer uses a ra oss Menu inside a ra-enterprise Admin (and instead of the ra ee Menu), the open/close feature doesn't work.

This PR forces the ra oss Menu to also handle widths based on open/closed status. And we decided that in the next major, it'd be better that the Menu alone handles the open/close status, not the sidebar.

@fzaninotto fzaninotto mentioned this pull request May 6, 2021
@fzaninotto
Copy link
Member

After testing, it appears that it's not possible to have a sidebar adapt to the width of its content. So we'll have to rely on the theme to let developers customize the sidebar width depending on the menu width.

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

Successfully merging this pull request may close these issues.

2 participants