-
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: PostCardPanel component. #59870
Conversation
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Size Change: +524 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
a3500ac
to
db722ca
Compare
packages/edit-post/src/components/sidebar/post-initial-panel/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/post-initial-panel/index.js
Outdated
Show resolved
Hide resolved
packages/edit-post/src/components/sidebar/post-initial-panel/index.js
Outdated
Show resolved
Hide resolved
8505f8d
to
d1a3af0
Compare
Hi @youknowriad, @ntsekouras your feedback was applied 👍 |
__( 'Last edited %s' ), | ||
humanTimeDiff( modified ) | ||
); | ||
const icon = CARD_ICONS[ type ] || templateInfo?.icon || pageIcon; |
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's a lot of inconsistencies and conditions in the way we compute "title", "description" and "icon" between post types. Can we track this. I'd like to have a unique way to retrieve these informations regardless of the post type. (Also this logic seems duplicated in multiple places for instance the document bar)
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.
➕ to this. We should have a way to retrieve this for any post type. Additionally the pageIcon
as default doesn't seem right, since we show this for post too and other post types. I'm wondering if we should fallback to the postType.icon.
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 added a new private selector getPostIcon, it should be able to be used in the cases where we need icons like the document bar, if we agree with this selector as a side PR I can propose one where the selector becomes widely used.
</VStack> | ||
</div> | ||
) } | ||
{ children } |
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 seems to be only used in templates and only in the site editor. I think we should remove this prop and show the areas here regardless of whether we're on site editor or post editor.
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 agree but to do that we need to move the TemplateAreas component and the selectors it uses to the editor package. It is going to be a big change I added a todo and we can do it in a separate PR (that will have a size similar to this one).
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.
Ok, sounds good.
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 better. I left some comments for now and others for later :)
d1a3af0
to
265ce8a
Compare
: sprintf( | ||
// translators: %s: Human-readable time difference, e.g. "2 days ago". | ||
__( 'Last edited %s' ), | ||
humanTimeDiff( modified ) | ||
); |
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 probably will close #39312. WDYT?
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.
Should we also show the last edited
for templates 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.
I added the last edited to the templates 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.
This is how it looks now with description and last edited date:
@jameskoster do we need any polishing 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.
Thanks for the ping. I pushed a couple of changes to match the latest styles on trunk.
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.
LGTM
Let's avoid the typographic widow here: We can do that editorially, or we can apply We should also update the block card to this new layout: Can happen separately. |
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.
Seems good to merge and continue iterating.
One detail I'd appreciate design feedback on is the inclusion of the icon. In #59689 it was omitted it for two reasons:
- It's already permanently visible in the document title bar, so is a little duplicative.
- It causes alignment awkwardness – IE do we align the text below with the title, or does it span full width? Each approach has a trade-off.
872d665
to
81364bc
Compare
Co-authored-by: jorgefilipecosta <jorgefilipecosta@git.wordpress.org> Co-authored-by: jameskoster <jameskoster@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org> Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org>
Part of: #59849
Adds a PostSidebarCard component that is reused across edit-site and edit-post. Uses the component on edit post making the inspector of edit-post similar to the inspector of edit-side.
Screenshots
Testing
Verify the inspector in edit-site is exactly as before.
Verify the inspector in edit-post works as expected and now contains the additional information at the top.