feature: allow params on resource menu item #1957
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Code Climate has analyzed commit 2656bf5 and detected 0 issues on this pull request. View more on Code Climate. |
|
Documentations changes - avo-hq/docs.avohq.io#114 According to the Contributing guide I should add some tags to the PR, but I don't know how. I can't change labels. When it comes to the Checklist point "I have commented my code, particularly in hard-to-understand areas" - I don't think there is part of the code that would require commenting. Correct me if I'm wrong. |
Paul-Bob
reviewed
Oct 3, 2023
lib/avo/menu/resource.rb
Outdated
| def fetch_params | ||
| Avo::ExecutionContext.new( | ||
| target: params, | ||
| resource: resource |
There was a problem hiding this comment.
Suggested change
| resource: resource | |
| resource: parsed_resource |
Paul-Bob
reviewed
Oct 3, 2023
lib/avo/menu/resource.rb
Outdated
| @@ -3,6 +3,7 @@ class Avo::Menu::Resource < Avo::Menu::BaseItem | |||
|
|
|||
| option :resource | |||
| option :label, optional: true | |||
| option :params, default: proc { {} } | |||
|
|
|||
| def parsed_resource | |||
| Avo::App.guess_resource resource.to_s | |||
There was a problem hiding this comment.
Suggested change
| Avo::App.guess_resource resource.to_s | |
| @parsed_resource ||= Avo::App.guess_resource resource.to_s |
|
Thank you for this amazing PR @bryszard! I let some tweaks suggestions and also added the tags so don't worry about it. |
Paul-Bob
approved these changes
Oct 9, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Fixes #1651
Docs https://docs.avohq.io/2.0/menu-editor.html
In this Pull Request I'm adding an option to pass
paramsto theresourcemethod on theMenuBuilder. It can be a Hash or a Proc that will return a Hash.Whatever is passed as
paramswill be added to the path of the menu item, when navigating to it.Before this PR, there was a POC on which @Paul-Bob helped me understand the scope of the changes to be done.
In the POC we've identified one caveat for future users of the feature - we don't support having multiple menu items for the same resource, but with different params. The reason is simple - if we have two menu items with the same path, they would be both highlighted as
activewhen visited. The workarounds for that using current libraryactive_link_forare imperfect and substituting it right now has no point.As @Paul-Bob stated here - we already have a feature to achieve the goal of having different filters for the same resource applied and these are the scopes.
With this feature it will be possible to add some default params to the menu item, which don't necessarily need to be the filters.
Checklist:
Video QA
2023-09-29.07-45-48.mp4
Manual review steps
Usersresource and check the pathManual reviewer: please leave a comment with output from the test if that's the case.