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.

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

Jump to bottom

Rename menu components #2996

Open
wants to merge 23 commits into
base: next
Choose a base branch
from
Open

Rename menu components #2996

wants to merge 23 commits into from

Conversation

juliawegmayr
Copy link
Contributor

@juliawegmayr juliawegmayr commented Dec 24, 2024 •
edited
Loading

Description

Rename all Menu components to better differentiate between comet and mui components:

  • Menu → MainNavigation
  • MenuItem → MainNavigationItem
  • MenuCollapsibleItem → MainNavigationCollapsibleItem
  • MenuContext → MainNavigationContext
  • MenuItemAnchorLink → MainNavigationAnchorLinkItem
  • MenuItemGroup → MainNavigationItemGroup
  • MenuItemRouterLink → MainNavigationRouterLinkItem

Acceptance criteria

Open TODOs/questions

  • Add changeset

Further information

@juliawegmayr juliawegmayr self-assigned this Dec 24, 2024
@juliawegmayr juliawegmayr force-pushed the rename-components-of-main-navigation branch 3 times, most recently from 68f98e9 to e5be58b Compare December 24, 2024 09:29
@juliawegmayr juliawegmayr mentioned this pull request Dec 24, 2024
Draft
@juliawegmayr juliawegmayr force-pushed the rename-components-of-main-navigation branch 2 times, most recently from 2774972 to e31290c Compare December 30, 2024 08:50
@juliawegmayr juliawegmayr force-pushed the rename-components-of-main-navigation branch from e31290c to 2461d1d Compare December 30, 2024 09:05
@juliawegmayr juliawegmayr marked this pull request as ready for review December 30, 2024 09:28
@auto-assign auto-assign bot requested a review from johnnyomair December 30, 2024 09:28
@juliawegmayr juliawegmayr requested review from jamesricky and removed request for jamesricky December 30, 2024 09:28
@juliawegmayr juliawegmayr marked this pull request as draft December 30, 2024 09:29
@juliawegmayr juliawegmayr marked this pull request as ready for review December 30, 2024 09:34
jamesricky
jamesricky reviewed Dec 30, 2024
View reviewed changes
.changeset/eighty-rockets-yell.md Outdated Show resolved Hide resolved
.changeset/eighty-rockets-yell.md Outdated Show resolved Hide resolved
docs/docs/migration/migration-from-v7-to-v8.md Outdated Show resolved Hide resolved
docs/docs/migration/migration-from-v7-to-v8.md Outdated Show resolved Hide resolved
jamesricky
jamesricky previously approved these changes Dec 30, 2024
View reviewed changes
jamesricky
jamesricky previously approved these changes Jan 2, 2025
View reviewed changes
johnnyomair
johnnyomair reviewed Jan 9, 2025
View reviewed changes
docs/docs/migration/migration-from-v7-to-v8.md Outdated Show resolved Hide resolved
packages/admin/admin/src/index.ts Outdated Show resolved Hide resolved
packages/admin/admin/src/mui/mainNavigation/CollapsibleItem.styles.ts Outdated Show resolved Hide resolved
packages/admin/admin/src/mui/mainNavigation/Context.tsx Outdated Show resolved Hide resolved
packages/admin/admin/src/index.ts Outdated Show resolved Hide resolved
@johnnyomair
Copy link
Collaborator

Now we have the terms main navigation and master menu for related, but different things. Might this confuse devs? @jamesricky what do you think?

@johnnyomair johnnyomair self-requested a review January 22, 2025 09:53
@jamesricky
Copy link
Contributor

Now we have the terms main navigation and master menu for related, but different things. Might this confuse devs? @jamesricky what do you think?

Since MainNavigation is in @comet/admin and the master menu in @comet/cms-admin and used in the project, I don't think it's very confusing... We could consider renaming MasterMenu into AppMenu or similar to make it a bit more obvious 🤔

@johnnyomair
Copy link
Collaborator

Since MainNavigation is in @comet/admin and the master menu in @comet/cms-admin and used in the project, I don't think it's very confusing... We could consider renaming MasterMenu into AppMenu or similar to make it a bit more obvious 🤔

Okay. Let's defer this for now.

@johnnyomair johnnyomair changed the title rename Menu components Rename menu components Jan 22, 2025
jamesricky
jamesricky reviewed Jan 22, 2025
View reviewed changes
.changeset/eighty-rockets-yell.md Outdated
@@ -27,4 +27,4 @@ To better differentiate between imports from `@comet/admin` and `@mui/material`,
- `MenuItemRouterLink` → `MainNavigationItemRouterLink`
- `MenuItemRouterLinkProps` → `MainNavigationItemRouterLinkProps`

Add `useMainNavigation()` hook to use instead of `MainNavigationContext`
Remove export of `MainNavigationContent`. Use `useMainNavigation()` hook instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think when mentioning the removal, it should use the previous name, as the new one didn't exist yet.
Also you can remove the mention of the rename for the removed item, as this won't be relevant.

Remove `MenuContext`, use the `useMainNavigation()` hook instead.

Copy link
Contributor

@jamesricky jamesricky Feb 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Also you can remove the mention of the rename for the removed item, as this won't be relevant.

Please also remove the mention of the rename of IMenuContext and MenuContext as this is now irrelevant as they are now internal.

@juliawegmayr @johnnyomair
Improve changeset
6740c82 
Co-authored-by: Johannes Obermair <[email protected]>
johnnyomair
johnnyomair previously approved these changes Feb 5, 2025
View reviewed changes
Copy link
Collaborator

@johnnyomair johnnyomair left a comment

Choose a reason for hiding this comment

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

LGTM. Conflicts need to be resolved though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@johnnyomair johnnyomair johnnyomair left review comments

@jamesricky jamesricky Awaiting requested review from jamesricky

At least 1 approving review is required to merge this pull request.

Assignees

@juliawegmayr juliawegmayr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@juliawegmayr @johnnyomair @jamesricky