Split out Views::MenuItem and Views::Menu #5163
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
varyonic
force-pushed
the
feature/menu-item-component
branch
from
a3c7dc6
to
46a8ca2
Compare
varyonic
force-pushed
the
feature/menu-item-component
branch
2 times, most recently
from
e27ae92
to
23db0c9
Compare
|
It's an improvement and should be easier to understand the components going forward. 👍 |
|
Rebased. |
varyonic
force-pushed
the
feature/menu-item-component
branch
from
23db0c9
to
3c88175
Compare
…ly in MenuItem component.
varyonic
force-pushed
the
feature/menu-item-component
branch
from
3c88175
to
80594f4
Compare
zorab47
approved these changes
Nov 7, 2017
varyonic
added a commit
to varyonic/activeadmin-rails
that referenced
this pull request
Oct 8, 2018
…omponent Split out Views::MenuItem and Views::Menu
Merged
javierjulio
added a commit
that referenced
this pull request
Dec 11, 2023
I do think the changes in #5163 are good but ideally the sorting should be done at the data layer, not in the DOM (Arbre). It has too though since the label, url, etc. have to rendered within the view context so its convenient to do there. I did this partial revert, because it would be easiest to use this within an extracted partial that the app host can customize to meet their needs. It is expected that a context has to be passed in for label, etc. since we have to render these blocks so no real way around that. The menu and menu item files haven't changed before #5163 for many years so I think the API is stable and solid for now.
javierjulio
added a commit
that referenced
this pull request
Dec 13, 2023
I do think the changes in #5163 are good but ideally the sorting should be done at the data layer, not in the DOM (Arbre). It has too though since the label, url, etc. have to rendered within the view context so its convenient to do there. I did this partial revert, because it would be easiest to use this within an extracted partial that the app host can customize to meet their needs. It is expected that a context has to be passed in for label, etc. since we have to render these blocks so no real way around that. The menu and menu item files haven't changed before #5163 for many years so I think the API is stable and solid for now.
javierjulio
added a commit
that referenced
this pull request
Dec 13, 2023
I do think the changes in #5163 are good but ideally the sorting should be done at the data layer, not in the DOM (Arbre). It has too though since the label, url, etc. have to rendered within the view context so its convenient to do there. I did this partial revert, because it would be easiest to use this within an extracted partial that the app host can customize to meet their needs. It is expected that a context has to be passed in for label, etc. since we have to render these blocks so no real way around that. The menu and menu item files haven't changed before #5163 for many years so I think the API is stable and solid for now.
javierjulio
added a commit
that referenced
this pull request
Dec 14, 2023
I do think the changes in #5163 are good but ideally the sorting should be done at the data layer, not in the DOM (Arbre). It has too though since the label, url, etc. have to rendered within the view context so its convenient to do there. I did this partial revert, because it would be easiest to use this within an extracted partial that the app host can customize to meet their needs. It is expected that a context has to be passed in for label, etc. since we have to render these blocks so no real way around that. The menu and menu item files haven't changed before #5163 for many years so I think the API is stable and solid for now.
javierjulio
added a commit
that referenced
this pull request
Dec 14, 2023
* Inline site footer styles with tailwind classes Now that we've moved this to a partial, no need to keep the styles as a component class. We did that for others to be able to easily override using CSS but no point in that since the footer is really meant for the user to completely customize without any defaults applied. * Extract sidebar to partials This allows users to customize the sidebar further if needed since these are simple components. The `attributes_table` method has been removed since it was a bad change. Sidebars can be used outside the show page context so by using `resource` internally that can lead to an unexpected error. This cleans up the API and matches what we have in the docs since we consistently show `attributes_table_for object, ...` usage. This also removes the unused sidebar methods from resource controller. This same module is duplicated and available as view helpers which are public so this is unnecessary. It has no references, nor any documentation. This adds test coverage for the sidebar helper which was missing. * Extract page header to partials For the action items, this incorporates the changes from #7085 to avoid unintended flex gap on the default action items when there are none to render. Since the conditional was within the block that was too late. It has to be added as if block like any other action item. * Extract site header to partial * Extract logout link to partial, remove configs We don't need the logout_link_method anymore since it's in a partial the host app is expected to update. Further the routes config we generate includes our Devise config which the host app can easily merge a sign_out_via override. * Revert nested submenus from #5994 We've already removed SASS so this is a revert of the behavioral change. It's not recommend for main navigations to have more than a single nested menu level. Further since the menu only rendered horizontally when the change was introduced, we now render the menu vertically as a drawer style component for mobile support. * Partial revert of #5163 for main menu extraction I do think the changes in #5163 are good but ideally the sorting should be done at the data layer, not in the DOM (Arbre). It has too though since the label, url, etc. have to rendered within the view context so its convenient to do there. I did this partial revert, because it would be easiest to use this within an extracted partial that the app host can customize to meet their needs. It is expected that a context has to be passed in for label, etc. since we have to render these blocks so no real way around that. The menu and menu item files haven't changed before #5163 for many years so I think the API is stable and solid for now. * Extract main menu to site header partial * Remove unused index_scopes view factory All that remains are the pages which will start to extract separately and then remove this view factory. * Inline styles in flash messages partial * Update button styles * Add dark mode toggle Defaults to system preference
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.
Tabbed navigation seemed unfinished. This refactoring moves all responsibility for rendering menus out of the MenuItem DSL structure to a new pair of view components. See also previous PRs #2031 and #3754.