-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 a new page/post button on publish panel #33276
Add a new page/post button on publish panel #33276
Conversation
Hi @thisissandip Sandip It looks great! Let's just remove the "Back to Dashboard" text link and it is done. We can then get a code review perhaps from @Mamaduka |
Hey @paaljoachim! I removed the Back to dashboard link. I guess it is ready for the code review! 😄 |
We can go with what Joen suggested here: #32910 (comment) (Joen also suggested removing the "Back to dashboard" text link.) As in giving space between the page address field and the Copy button. |
Are we done? Any more design comments? |
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.
Thanks for looking into this @thisissandip !
Some UX feedback here, moving the copy button up looks like it crowds the post address field. So in some cases we'll normally only see our site domain.
The width of the copy button can also change when the text label is updated from copy
to copied!
, which might be worse in other languages. This is more pronounced when we put this next to the post address field (there's less button padding).
Custom post types can have long names, so we might need to add a more generic label like "add item".
Before:
before.mp4
After:
after.mp4
Custom Post Type:
custom.post.type.mp4
To help move this forward, lets:
- Drop sentence case changes in this PR (We can still pursue this in a more general follow up discussion). If possible it'd be great if we could avoid magic numbers in CSS styling too.
- Get additional design feedback, cc maybe @kellychoffman or @jasmussen ?
@@ -91,9 +96,17 @@ class PostPublishPanelPostpublish extends Component { | |||
render() { | |||
const { children, isScheduled, post, postType } = this.props; | |||
const postLabel = get( postType, [ 'labels', 'singular_name' ] ); | |||
const viewPostLabel = get( postType, [ 'labels', 'view_item' ] ); | |||
const viewPostLabel = ConvertToSentenceCase( |
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.
Let's drop the sentence case changes here for now.
This is part of a larger discussion, but instead of doing this in JS, I'm thinking we should converge on a standard for sentence case or title case, and update the i18n labels instead.
https://core.trac.wordpress.org/ticket/49616
cc @sixhours @swissspidy or anyone else interested in this.
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.
Agreed. This function should be removed.
This only really works well in English but not necessarily in other languages.
+1 to label the button "add item" for custom post types.
That's a good point, @gwwar… could we solve this problem using a clipboard icon instead of the text? Or maybe just the tick icon when the action is done, like GitHub does when you grab the URL of a repo to clone it. Another idea could be adding a message below the input field, like in the Pattern Directory. |
@gwwar I have made the required changes let me know if this looks good! 😄 |
This looks really close! It's good to use "Add item" for custom post types, but it may be better to stick with named types like "post"/"page" for the built-ins. We'll end up with mixed label usage otherwise with: "View post/Add item"
Besides that, technically this looks fine to me. What's left is a +1 for Design approval ✅
@javierarce I think something like that could work ✨ Do you think we should leave that exploration for a follow up issue? |
: __( 'Copy Link' ) } | ||
</CopyButton> | ||
<Button variant="secondary" href={ addLink }> | ||
{ __( 'Add Item' ) } |
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 should be coming from the post type labels.
Why should this be good? Using the correct post type label like was done before da70034 makes more sense. If you need a new post type label you should just patch it / add it. |
@swissspidy What I mean is the final output text. Eg existing behavior for known post types (using the post type label like before), "View post/Add post" and "View item/Add item" for cpts. I can of course defer to folks if y'all feel strongly, but what can happen is that labels are mismatched in the CPT case with "View item/Add new cpt name" |
Oh sorry, @swissspidy is right here with the label usage 👍. I must have some mixed label definitions with the plugin I was testing with. Beside changing that back, this still needs a design +1 |
I have updated the code to use the correct post type label for the Add New button |
Ok, I think this is fine from a design perspective, but I have one final observation: I think we should make the view button the primary action to make the hierarchy of the actions more clear.
@gwwar Yes, I'll create a new issue for that. |
@javierarce so to confirm the two screens: add an
|
@gwwar oh, yes, sorry… in that case the "add new" button will be the primary action, so we need to add the class there too. |
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.
Tests well for me @thisissandip! I'll leave this open for a day in case others had any other feedback. Will merge next week.
Thanks for the reviews @javierarce @swissspidy ✨
Nice work @thisissandip Sandip! Thank you very much for testing @gwwar Kerry! I am testing on a local dev site of a client site that uses a CPT named Nettmagasin. One thing I noticed while reading Joen's comment here: "While this was titlecase before, we should actually be using sentence case everywhere, so since this PR touches those parts it might be nice to go in and fix that. So "View Post" → "View post", "Add New Post" → "Add new post" I noticed the title case is correct here: That's what I noticed. |
It will show up with whatever label the post types has been registered with. |
If we'd like sentence case for English, this needs to be updated in the post labels themselves and also tested for other UI areas that also use them. Note that sentence case doesn't make sense for some i18n locales. See also: https://core.trac.wordpress.org/ticket/49616 |
Since it looks like there's no major feedback, let's try to land. We can see if there's any additional feedback during the RC and address it in follow up PRs/issues. |
Neat change! I was looking at this and thought same as @gwwar:
It was the case already previously without copy button — in screenshot below showing the URL isn't helpful at all: As a follow-up, have you considered some other format than input? Few ideas:
Super rough mock, doesn't look great but conveys the idea: Also worth considering "Post name is now live" text kinda doubles as preview link already? |
@gwwar @thisissandip Would be greatly appreciated if you could add |
Will do in the future! I forgot to check the labels. |
Fixes #32099
Description
Adds a create "New Page/Post" button at the end of the publish check panel.
How has this been tested?
Screenshots
post_publish_button.mov
Types of changes
New nonbreaking feature
Checklist: