Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.08%. Comparing base (75acaf3) to head (e72df6c).
⚠️ Report is 1 commits behind head on master.

@@           Coverage Diff           @@
##           master    #8225   +/-   ##
=======================================
  Coverage   99.08%   99.08%           
=======================================
  Files         139      139           
  Lines        4045     4045           
=======================================
  Hits         4008     4008           
  Misses         37       37           

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

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.

@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.

@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.

@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 adds the link to the resource.

@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.

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

@javierjulio Sorry to ping again, but this is a really wanted feature and honestly a blocker for upgrading

@javierjulio ?

Hi guys! I bumped into the same issue #8245

Do u need any help with the PR? I'd be really happy to get this thing merged

@hasghari we will merge #8709 first to bump Tailwind from v3 to v4 then we'll come back to this. Nothing to do for now until #8709 is merged. I'm not sure if the major version affects your changes, it may not, but feel free to test yourself if you like. Once #8709 is merged, I'll test this and suggest any changes. Both updates are planned to go in the next beta release. Thank you for your patience.

@hasghari we will merge #8709 first to bump Tailwind from v3 to v4 then we'll come back to this. Nothing to do for now until #8709 is merged. I'm not sure if the major version affects your changes, it may not, but feel free to test yourself if you like. Once #8709 is merged, I'll test this and suggest any changes. Both updates are planned to go in the next beta release. Thank you for your patience.

@javierjulio Thanks, I've rebased this PR branch against master.

@javierjulio Thanks for the feedback. I've updated the code to address the issue you raised.

@hasghari my expectation here was that for the original case, that the parent menu item was one button. I pushed some updates so that case remains, while the new one addresses the split button setup: a link for the page destination and a toggle button for the nested menu. This also applies the desired default styles for all cases. Please give this a try and let me know how it works. If all clear, I'll approve and merge. We'll cut a release with this update. Thanks.

@hasghari my expectation here was that for the original case, that the parent menu item was one button. I pushed some updates so that case remains, while the new one addresses the split button setup: a link for the page destination and a toggle button for the nested menu. This also applies the desired default styles for all cases. Please give this a try and let me know how it works. If all clear, I'll approve and merge. We'll cut a release with this update. Thanks.

@javierjulio Looks good to me, thanks for making that change.