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

Add support for menu subsubitems #1234

Merged
merged 29 commits into from
Mar 4, 2024
Merged

Conversation

jomunker
Copy link
Contributor

@jomunker jomunker commented Aug 21, 2023

depends on #1233

This PR adds support for menu subsubitems (third level items).

Screenshots

image
image

@jomunker jomunker self-assigned this Aug 21, 2023
@jomunker
Copy link
Contributor Author

jomunker commented Aug 21, 2023

  • add changeset

@thomasdax98 thomasdax98 requested review from jamesricky and removed request for thomasdax98 August 22, 2023 21:11
@jomunker jomunker changed the base branch from next to collapsed-menu September 27, 2023 08:30
@jomunker jomunker requested a review from johnnyomair January 3, 2024 10:22
@jomunker jomunker requested a review from johnnyomair February 7, 2024 11:26
@johnnyomair
Copy link
Collaborator

I'll review this once #1233 has been merged. Setting as draft for now.

@johnnyomair johnnyomair marked this pull request as draft February 21, 2024 15:44
Base automatically changed from collapsed-menu to feature/menu-rework February 22, 2024 15:10
@johnnyomair
Copy link
Collaborator

@jomunker the base PR has been merged, please update this branch and solve conflicts so we can review it again, thanks!

@jomunker jomunker marked this pull request as ready for review February 22, 2024 18:13
@jomunker jomunker requested a review from johnnyomair February 22, 2024 18:13
Copy link
Contributor

@jamesricky jamesricky left a comment

Choose a reason for hiding this comment

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

Could you please add an example of this feature to the dev-stories?
Either in a new story or maybe in the existing menu-story: packages/admin/admin-stories/src/admin/mui/Menu.tsx

This would be very helpful for review and for future changes/debugging of this feature.

@jamesricky
Copy link
Contributor

 @jomunker the implementation seems to differ from the design:

  • The selected sub-sub items should not have a blue background but the text and the border on the left should be blue instead.
  • The left border of the last item should extend all the way to the bottom of the item, like the border on the first item extends all the way to the top.

Are these things already implemented in one of the follow-up PRs?
Or do I maybe have an outdated design?

Current implementation Design
Current implementation Design

@jomunker
Copy link
Contributor Author

@jomunker the implementation seems to differ from the design:

  • The selected sub-sub items should not have a blue background but the text and the border on the left should be blue instead.
  • The left border of the last item should extend all the way to the bottom of the item, like the border on the first item extends all the way to the top.

Are these things already implemented in one of the follow-up PRs? Or do I maybe have an outdated design?

Current implementation Design
Current implementation Design

Seems like i fixed the design in the follow up PR. See the following screen recording. Sorry for the confusion with the follow up PRs.

Screen.Recording.2024-02-29.at.10.21.42.mov

@jamesricky
Copy link
Contributor

Seems like i fixed the design in the follow up PR. See the following screen recording.

The bottom part of the border on the last item is still missing in the video.
Can you please add that? In a follow-up PR is fine.

Screenshot 2024-02-29 at 15 58 24

@johnnyomair johnnyomair merged commit 9c4b7c9 into feature/menu-rework Mar 4, 2024
2 checks passed
@johnnyomair johnnyomair deleted the menu-subsubitems branch March 4, 2024 10:01
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.

3 participants