-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
FSE: Add basic template information to editor header #25320
Conversation
label={ _x( | ||
'Add block', | ||
'Generic label for block inserter button' | ||
<div className="edit-site-header_start"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes in this file add three wrapper divs around the content of the header. One for the "start", "center", and "end". The start and end divs have flex: 1
. This way, the center div is always centered in the middle of the viewport regardless of the width of the content inside the start/end divs.
packages/edit-site/src/components/header/document-actions/index.js
Outdated
Show resolved
Hide resolved
Size Change: +539 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
return ( | ||
<div className="edit-site-document-actions"> | ||
{ primaryText ? ( | ||
<Dropdown |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where we've landed on this being a dropdown; I think it makes a lot of sense for the document/template label to open some settings, but I'm not so sure about the template part label. In some designs the template part label could be useful as a way to select the template part itself. This could confuse the interactions between dropdown and selection.
For now, I'd suggest we move forward with just the labels as indicators of what template and template part are being edited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In some designs the template part label could be useful as a way to select the template part itself.
If the label already shows the name of the template part, is it not already selected or hovered? And in the case of being hovered you couldn't hover it and click the name in the top-bar at the same time so... I guess I dont understand how clicking the name of the Template Part here would act to select it (as it would always already be selected)?
Edit - I guess that would be to select the template part itself if we already have one of its child blocks selected? 🤔 That seems a bit odd, I wonder if normal users would actually figure that out and utilize it? But its definitely something we could make work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure where we've landed on this being a dropdown; I think it makes a lot of sense for the document/template label to open some settings
So what should it open for controlling the settings then? I thought the current design mocks for these settings were a dropdown?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be best to focus on showing the template part being edited first — and then add the interaction in another PR. I'll defer to @dubielzyk here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that sounds fine to me. I used the dropdown because I thought the document settings menu would be activated on click of this "area," and based on current designs that does at least sort of look like a dropdown menu 😅
Totally on board with keeping just the labels for the time being.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep! Per:
Top label is template or page/post
I think this opens a can of worms conceptually, because if we go with "page," we're starting to think about the site editor from a user's page-first perspective rather than a technical person's template-first perspective. To be clear, it's easy to change the variable rendering this technically :D
I'm mostly convinced by the argument to think about the site editor from a page-first perspective.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I 100% agree that we should go page/post first. I think we only show the template if they use an entry-point that is about editing a specific template.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we only show the template
right, and to be clear, the template would get "shown" in the editor either way, but it would be more of the focus there
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That does make sense. I think, as we discussed in the meeting, this is tricky because there are no entrypoints currently which let us infer if the user wants to mainly edit a template or a page. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice job! These changes are compact, straightforward, and I think they'll serve as a great point of discussion for design.
As far as the code goes, I don't see any red flags or areas of concern. It also seems like the work here can be adapted very easily to the new mocks proposed by Shaun. 👍
cc @shaunandrews updated with vertical design: |
packages/edit-site/src/components/header/document-actions/style.scss
Outdated
Show resolved
Hide resolved
3263876
to
63d1079
Compare
In 202d122, I added the animation from the design, and also corrected the spacing to match this comment. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking good and behaving well to me!
As noted in TODO
we don't have the child selected state covered, but I would be happy with this being addressed in a follow-up (potentially along with the hover label).
202d122
to
f4927d4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This works well and looks good to me!
I noticed the template title isn't capitalized for index/singular on my end. Do we need to use a title/label instead of the slug? (not to block this, however)
nice catch! Currently the title is equal to the slug, so it also isn't capitalized. I'm personally inclined to leave it as is since we'll probably switch to use the page context soon. Plus, the index slug is probably the most accurate thing to use at this point since we don't have a solid system for having a "display name" for a template |
Description
Adds a basic "document actions" component which is rendered in the center of the FSE editor header. Note: the string rendered in the title is TBD. Currently it uses the template name, but we are also thinking that the page name could be more useful / easier to understand.
Also, when a template part block is directly selected, it will show its label as secondary text in the header area. We are hoping to expand this functionality to include hover, and to handle if the parent of the selected block is the template part.
This is also meant to serve as a starting point for adding a document settings dropdown to the header.
to do :
Work towards #25085. cc @dubielzyk
How has this been tested?
locally in edit site
Screenshots
A friendly reminder that eventually, the "index" text in the left will go away.
Types of changes
Enhancement
Checklist: