Make it easier to customize admin and main layouts #5063
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
javierm
force-pushed
the
admin_menu_links
branch
3 times, most recently
from
562558b
to
dbca3b5
Compare
javierm
force-pushed
the
refactor_layout_components
branch
from
e1d4111
to
50f1926
Compare
javierm
force-pushed
the
refactor_layout_components
branch
5 times, most recently
from
ad7f4f9
to
45a8447
Compare
This way it's easier to refactor it. Note we're using `with_request_url` in the tests because the component renders the locale switcher, which needs a URL in order to work. This doesn't affect whether we're in the management section or not.
This way we remove the duplication in the layouts which had these links. Since we're now passing the `current_user` option to partials in all cases, IMHO the code is easier to follow if we use the `Layout::NotificationItemComponent` instead of its partial.
This way it'll be easier to decide when they should be rendered. In order to be consistent, we're using the `Layout` module for both components; previously, the navigation partial was in the `shared` folder while the footer partial was in the `layout` folder, which IMHO didn't make much sense.
In the management section, `current_user` is the user impersonated by the manager. We were deciding whether to show the admin menu depending on the privileges of the current user, but this menu should be shown according to the privileges of the manager who is impersonating the user. We're doing a similar (very subtle) change in the login items. We were rendering the `login_items` partial passing `current_user: user`. However, inside this method, we were using `user_signed_in`, which ignored the `current_user` we were passing. The result was always the same expect in tests where we manually sign in users, but we're changing it anyway in order to reduce confusion.
javierm
force-pushed
the
refactor_layout_components
branch
from
45a8447
to
6fbaccb
Compare
This menu isn't rendered in this case, so the "menu" button to toggle it did nothing.
We're trying to be consistent; in the past, we had the partials "shared/admin_login_items", "layouts/notification_item" and "devise/menu/login_items". Now we're moving all these partials to components in the `Layout` namespace.
This way it's easier to customize this menu.
javierm
force-pushed
the
refactor_layout_components
branch
from
6fbaccb
to
967bb4d
Compare
taitus
approved these changes
Jan 16, 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.
References
Objectives