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

Link to parent menu item #8225

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

hasghari
Copy link
Contributor

@hasghari hasghari commented Jan 9, 2024

Currently when rendering a nested menu, if the parent menu item links to a url, the link is not rendered for the parent item and there is no way to navigate to it.

Here is the nested menu appearance for light mode and dark mode

I'm not a designer by any means so I'm open to changing the UI.

@hasghari hasghari force-pushed the main-menu-navigation branch 2 times, most recently from 4775e2f to 358317b Compare January 9, 2024 23:54
Copy link

codecov bot commented Jan 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.10%. Comparing base (ef7a482) to head (6ed5a79).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #8225   +/-   ##
=======================================
  Coverage   99.10%   99.10%           
=======================================
  Files         140      140           
  Lines        4018     4018           
=======================================
  Hits         3982     3982           
  Misses         36       36           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

lib/active_admin/menu.rb Outdated Show resolved Hide resolved
@hasghari hasghari force-pushed the main-menu-navigation branch 2 times, most recently from 8349e5e to b36fd5a Compare January 11, 2024 14:17
@hasghari hasghari force-pushed the main-menu-navigation branch 3 times, most recently from def150a to 48f2861 Compare January 13, 2024 13:47
@javierjulio
Copy link
Member

javierjulio commented Jan 13, 2024

This is made to sound like it's considered a bug but from what I know this is intentional. It wasn't supported previously. Also, I don't think it's a good default for the parent menu item to have a link and a separate toggle. I will not accept this change, sorry.

Although, you should be able to make this change for your own implementation, that's why we have the navigation in a partial so you can customize it. For that, it seems you would need a Ruby method update to support that case which is something I would accept. You're welcome to contribute just that change here.

The JS is only meant as a default, out of the box. You can supply your own JS for custom menu behavior by just changing the element id and working off that. Perhaps, in the future we can expand on the JS so you don't have to import/load what you end up replacing or don't use. I would appreciate any help or suggestions around that for a v4 release.

@hasghari
Copy link
Contributor Author

@javierjulio Actually, this was supported in version 3. Take a look at the following snippet in the menu.feature spec:

Scenario: Adding a resource as a sub menu item
  Given a configuration of:
  """
    ActiveAdmin.register User
    ActiveAdmin.register Post do
      menu parent: 'Users'
    end
  """
  When I am on the dashboard
  Then I should see a menu item for "Users"
  And the "Posts" menu item should be hidden
  When I follow "Users"
  Then the "Users" menu item should be selected
  And I should see a nested menu item for "Posts"

Without the change in this pull request, there is no way for you to navigate to the User resource. I think this is a bug.

@javierjulio
Copy link
Member

@hasghari sorry, I don't use that feature. I'm not familiar with it at all. Nor do we seem to have it in the generated app for development or test. Often times these features are latter discovered unless built in to the development app. It's not possible for us to know all of them. Can you please modify the dev app that is generated locally to use that feature and share what changes you had to do to get that? I'm not able to replicate this. I don't have any idea what it would even render. Thank you.

@hasghari hasghari force-pushed the main-menu-navigation branch 3 times, most recently from 93cf695 to 5b59de1 Compare January 14, 2024 03:38
@hasghari
Copy link
Contributor Author

@javierjulio I added a new nested resource in the generated app called Profile. When you run rake local server, you will see the menu item for "Profiles" nested under "Users". Without this pull request, the "Users" item does not navigate anywhere. This pull request add the link to the resource.

@tagliala tagliala linked an issue Jan 29, 2024 that may be closed by this pull request
@mgrunberg
Copy link
Contributor

@javierjulio Actually, this was supported in version 3. Take a look at the following snippet in the menu.feature spec:

Scenario: Adding a resource as a sub menu item
  Given a configuration of:
  """
    ActiveAdmin.register User
    ActiveAdmin.register Post do
      menu parent: 'Users'
    end
  """
  When I am on the dashboard
  Then I should see a menu item for "Users"
  And the "Posts" menu item should be hidden
  When I follow "Users"
  Then the "Users" menu item should be selected
  And I should see a nested menu item for "Posts"

Without the change in this pull request, there is no way for you to navigate to the User resource. I think this is a bug.

The PR LTGM, I use this feature so thanks for fixing it.
I'm wondering why the feature test didn't fail. @hasghari, do you know?
I think the PR should also fix that.

@henrikbjorn
Copy link

@javierjulio Is it possible to get a fix for this in, hopefully we have changed your mind on this feature :)

Currently when rendering a nested menu, if the parent menu item
links to a url, the link is not rendered for the parent item
and there is no way to navigate to it.
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.

4.0.0.beta4 nesting in menus
5 participants