-
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
Editor: Make revisions more prominent #62323
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 Unlinked AccountsThe following contributors have not linked their GitHub and WordPress.org accounts: @ppolo99, @smithjw1. Contributors, please read how to link your accounts to ensure your work is properly credited in WordPress releases. 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: +102 B (+0.01%) Total Size: 1.74 MB
ℹ️ View Unchanged
|
Jay is out on a break, but we can ask @richtabor for a quick take too. I'm wondering, could it just be the "Last edited 6 days ago" that shows the revisions? Alternately, I'd lean this way: I.e. another row. |
Flaky tests detected in 6f4a512. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/9381447424
|
Agreed on showing it as an additional row. That feels like the proper weighting and like a better design choice. |
6f4a512
to
c03bfc3
Compare
I like @jasmussen's design a lot more than my quick idea (not surprising to me). I did some quick testing in the playground using chrome and this is looking good. I see that the revisions update upon a save and that they appear after there is a second revision. Clicking them takes them to the revisions screen. I think that overall this reduces the prominence of revisions vs wp6.5 but not at the expense of making them hard to find or making the number of revisions only available on the revisions screen. Thank you! |
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.
c03bfc3
to
97d22df
Compare
I've a same question. |
Personally I think it's fine to have it in both places, but that's something designers could know better. From the technical aspect, this action is also used in Data Views, so we would need to check the APIs for something like context, we've been discussing. |
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.
Codewise changes look good. The action can be removed separately based on design feedback.
* | ||
* @return {Component} The component to be rendered. | ||
*/ | ||
function PostLastRevision() { |
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 looking at some other panel refactorings; they didn't keep the original versions. Example: #61357.
Should we do the same 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.
I don't think it's the same case because here we're not changing the entire panel (PostLastRevisionPanel
) that is exported. So consumers of the above panel, should have the 'old' PostLastRevision
component.
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.
If the policy is to keep the old components intact, then we might need to revisit some of the refactoring, like #61357. Because PostDiscussionPanel
has a new structure.
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 don't think there is a clear policy about that, but I think the changes should not be disruptive in the sense that they would make sense in other designs too. An example would be if we change a control and the exporting component is just that. In this case IMO it's fine to not keep the old version. In this PR for me it made more sense to keep the old one as it matches better when used inside PostLastRevisionPanel
with the panel body and we actually don't use PostLastRevisionPanel
anymore in our codebase.
Can you share the components you're concerned with the discussions panel? Also cc @jorgefilipecosta for visibility.
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.
My concern is that the discussion panel consumers expect it to render PanelBody > PanelRow
. Now, they'll get a different design.
I personally don't mind either path for changes as long as they're consistent.
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 know they get a different design and might be a bit different, but in my mind they still get the whole functionality (the whole panel). If I had to update the panel here for revisions personally I'd follow the same approach with the discussions panel. This is subjective though..
I'd remove the link from the menu. |
This is much better, cheers for actioning. |
Let's land this and explore how to remove the action in the editor context separately in actions API. |
Unlinked contributors: ppolo99, smithjw1. Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: creativecoder <grantmkin@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: aaronjorbin <jorbin@git.wordpress.org> Co-authored-by: hrkhal <hrkhal@git.wordpress.org>
Unlinked contributors: ppolo99, smithjw1. Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: creativecoder <grantmkin@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: aaronjorbin <jorbin@git.wordpress.org> Co-authored-by: hrkhal <hrkhal@git.wordpress.org>
Unlinked contributors: ppolo99, smithjw1. Co-authored-by: ntsekouras <ntsekouras@git.wordpress.org> Co-authored-by: creativecoder <grantmkin@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: jasmussen <joen@git.wordpress.org> Co-authored-by: annezazu <annezazu@git.wordpress.org> Co-authored-by: aaronjorbin <jorbin@git.wordpress.org> Co-authored-by: hrkhal <hrkhal@git.wordpress.org>
This was cherry-picked to the wp/6.6 branch. |
What?
Fixes: #62041
There is some feedback in the issue that the revisions link and revisions number should be a bit more prominent, besides the post action in the ellipsis menu. This PR adds this info in post summary like suggested in the below comment.
Testing Instructions
Screenshots or screencast