Site Editor: Only mark the 'Site' menu item active when editing a home template #42807
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
Mamaduka
added
[Type] Bug
|
Size Change: +45 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
|
Thanks for taking this on @Mamaduka :) One problem with this execution is that if you have a static homepage (which uses the Page template), then the 'Site' menu item appears active if you open the Page template which feels a bit strange:
Ideally 'Site' should only be active when:
The issue on trunk is that Site also becomes active when you select a template. |
|
@jameskoster, I'm not able to reproduce the issue. For example, if I visit P.S. The homepage will say "Page" in the top toolbar when using a static template. |
|
Hmm, you're right. Maybe my build didn't complete before I refreshed 🙈 Seems to be working now :) I did notice another issue (although perhaps not something to fix here). If you click 'Site' from the templates list it will collapse the nav panel and load the site homepage. However if you click 'Site' whilst editing a template then the nav panel remains open. I wonder if these behaviours should be consistent? Happy to open a new issue if you prefer :) |
|
@jameskoster, I believe that's intentional and not related to these changes. gutenberg/packages/edit-site/src/components/app/index.js Lines 51 to 59 in 5bfc80b |
|
@Mamaduka thanks for looking into that, I guess we can address that point separately – I'll open an issue. Returning to this PR, I see only one remaining scenario to consider – the front-page template. Since that will always resolve to display the homepage regardless of reading settings, perhaps we should highlight the 'Site' menu item when it's being edited. What do you think? |
Mamaduka
force-pushed
the
try/site-menu-active-state
branch
from
4fff454 to
ef74809
Compare
|
@jameskoster, that's a good point. I've updated the logic to account for P.S. I've also realized that I made a mistake in the |
Mamaduka
commented
Aug 2, 2022
Comment on lines
43
to
48
| // The existence of `'front-page` supersedes very other settings. | ||
| const homeTemplateType = | ||
| params.postId?.includes( 'front-page' ) || | ||
| params.postId === homeTemplate?.postId | ||
| ? 'site-editor' | ||
| : undefined; |
There was a problem hiding this comment.
@kevin940726, do you think it's worth extracting this into a utility function like getIsListPage?
There was a problem hiding this comment.
Oops, only just saw this now. I don't have a preference, but usually I'd only do that if it's repeated multiple times.
There was a problem hiding this comment.
I do the same. I will extract it into a function when there's a need.
Mamaduka
force-pushed
the
try/site-menu-active-state
branch
from
ef74809 to
837df96
Compare
Mamaduka
force-pushed
the
try/site-menu-active-state
branch
from
837df96 to
b7246c9
Compare
|
I rebased and resolved merge conflicts. I think the remaining question is about a11y. To quote @kevin940726 - #36821 (comment)
|
|
I don't think I understand the problem 🤔 |
|
@jameskoster, If I understand correctly, the problem is missing |
|
It seems justified to me that a navigation menu can entertain a state where there is no currently-selected item. So I don't think that should be a problem. It would be good to get a11y sign-off from someone with more expertise though :D |
13 tasks
|
@jameskoster, should we merge this? Will the same active menu logic apply once the Browser Mode is merged? |
|
It's not yet clear whether the 'Site' or 'Browse' menu item will be needed long term (#44770 for context). But I don't think that should stop us merging this PR, since it's a clear enhancement on the current behaviour. |
Mamaduka
force-pushed
the
try/site-menu-active-state
branch
from
b7246c9 to
9921c2d
Compare
|
Thanks, @jameskoster 🙇 I just rebased and will merge after all checks are green. We'll probably get more feedback after this is merged into the plugin. |
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.
What?
Fixes #36821.
PR marks the "Site" menu item active only when editing a home template.
Why?
See #36821.
How?
Compares if template ID from navigation and editor settings match.
Testing Instructions
Screenshots or screencast