-
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
Add very basic template information dropdown #25757
Conversation
Size Change: +568 B (0%) Total Size: 1.19 MB
ℹ️ View Unchanged
|
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 looks like a great starting point!
I also am digging that anchorRect
, I didn't notice that was a thing previously. 😁
63c35c9
to
96cdaef
Compare
packages/edit-site/src/components/header/document-actions/index.js
Outdated
Show resolved
Hide resolved
30e1caa
to
27790ca
Compare
@@ -0,0 +1,7 @@ | |||
.edit-site-template-details__label { |
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 feel like there should be existing styles for this somewhere 🤔
packages/edit-site/src/components/header/document-actions/index.js
Outdated
Show resolved
Hide resolved
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.
Two things I've noticed while testing:
- There is no margin between the title and button border when dropdown is selected.
- The template title is now displaced to the left a bit, so when we select the template part and show its title there, the animation is not as good. Previously it was just sliding up, but now there is also the initial jump to the right too. I'm thinking that we could show the chevron when template part is selected too, but just update the content/context of the dropdown?
<p className="edit-site-template-details__heading"> | ||
{ __( 'Template details' ) } | ||
</p> | ||
<p>{ `${ __( 'Name' ) }: ${ template.slug }` }</p> |
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.
We should link this to corresponding item in navigation sidebar. Clicking on it should open the menu and activate the item. This is fine to do in a follow-up though.
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.
Ah, I see. That makes sense, I vote to do it in a follow-up :)
fd85e84
to
a1b5d23
Compare
Added a bit of margin to fix that:
I think the designs so far have been to avoid this. 🤔 Unless we add the dropdown while the template part is selected, there's no way to avoid the jump. I don't think there is any extra context we could show -- I'd think the dropdown would still be beside the template name, and shouldl still show template info |
cc @david-szabo97 in e52fe395a0f48682aaf2fc8b8de61c327347255e, I refactored the template information calculation so we can reuse it. The only weird thing is that I'm unsure where to put the constants file now. |
2acb88a
to
e52fe39
Compare
755f5ff
to
fbde9ef
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.
Tested in Chrome, Safari, Edge, IE11, and Firefox:
- Template switching with the navigation sidebar and noting that the label changes appropriately
- Template details are correct
- Template part labels change appropriately
+1 when we resolve the discussion about specs 🙂
94d3f14
to
a47a474
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 all looks good and works well for me!
Im just wondering about the dropdown not being usable after a template-part is selected. Was this how design wanted it or was that more unspecified?
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.
Looks like that is as designed. Nice job 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.
It looks like Github has degraded performance. Feel free to merge when your new commit is received by the platform 😅
af34640
to
8a26fed
Compare
Can we then preserve the dropdown while template part is selected, but still show the same info? |
yeah, we definitely can do that fairly easily, just a matter of design :) cc @shaunandrews |
Description
Adding a very basic foundation to iterate on for the template info dropdown.
How has this been tested?
Locally, in edit site.
Screenshots
Types of changes
New feature.
Checklist: