Skip to content

Revert "Revert "Allow nested sub-menus in the ActionMenu component"" - #3662

Closed
HDinger wants to merge 1 commit into
primer:mainfrom
opf:add-second-level-menu-again
Closed

Revert "Revert "Allow nested sub-menus in the ActionMenu component""#3662
HDinger wants to merge 1 commit into
primer:mainfrom
opf:add-second-level-menu-again

Conversation

@HDinger

@HDinger HDinger commented Aug 29, 2025

Copy link
Copy Markdown
Contributor

In #3638 it was said, that there were some integration issues with the original implementation of the multi-level menu which is why it was reverted. This PR aims to fix those issues.

This reverts commit 668a48f.

@HDinger
HDinger requested review from a team as code owners August 29, 2025 06:27
@HDinger
HDinger requested a review from mperrotti August 29, 2025 06:27
@changeset-bot

changeset-bot Bot commented Aug 29, 2025

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: f482125

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@primer/view-components Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@HDinger

HDinger commented Aug 29, 2025

Copy link
Copy Markdown
Contributor Author

Hi @jonrohan

Sorry for the incoviniences causes by our changes. If you give us some more details about the issues you ran into, we'd be happy to help fixing them.

cc/ @bsatarnejad @myabc

@lesliecdubs
lesliecdubs requested a review from jonrohan September 2, 2025 19:52
@HDinger

HDinger commented Oct 1, 2025

Copy link
Copy Markdown
Contributor Author

Hey everyone :) Any updates on what the problems were you were facing with the multi-level menu?

@jonrohan jonrohan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks @HDinger and @camertron for this contribution.

However we won’t be able to move forward with it, and I want to give you clear context on why.

GitHub’s frontend architecture has evolved, and we now deploy our Rails code and our frontend assets through separate pipelines. This separation is intentional it improves reliability, safety, and velocity but it also means we can’t guarantee that code generated from Rails and code from TypeScript assets will land in production at the same time.

Because of that, any Rails + JS/CSS changes need to be written with full backwards- and forwards-compatibility in mind. In practice, old Rails + new JS, new Rails + old JS, and new Rails + new JS all need to work together without breakage. PVC’s mixed Rails/JS architecture makes this particularly challenging.

Additionally, GitHub is focusing future investment on Primer React to support the product’s long-term frontend needs. As a result, we’re not taking on significant changes to PVC at this time. We’ll be sharing more about this direction in this repo soon.

You’re absolutely welcome to fork the repo and adapt it for your use cases, and we appreciate the thought and work that went into this contribution.

@HDinger

HDinger commented Nov 14, 2025

Copy link
Copy Markdown
Contributor Author

Hi @jonrohan

Thanks for the update 👍 Totally understand that architectural decisions like these are necessary as the platform evolves, and it makes sense that mixed Rails/JS changes need to remain fully backwards- and forwards-compatible under your deployment model. No worries on our side — since we already maintain a fork, we can easily continue adapting things to our needs.

We mainly opened the PR because we benefit greatly from the work you’ve put into this project, and we wanted to give something back. So thank you again for the clarity, and for the foundation you’ve built — we really appreciate it. 🙇

@HDinger HDinger closed this Nov 14, 2025
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.

2 participants