Skip to content
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: Add link to revisions #12197

Merged
merged 27 commits into from
Sep 13, 2022
Merged

Editor: Add link to revisions #12197

merged 27 commits into from
Sep 13, 2022

Conversation

timarney
Copy link
Contributor

@timarney timarney commented Aug 31, 2022

Context

In Gutenberg there's a link to the revisions comparison screen in the document tab It would be nice to do the same in the story editor as well, maybe in the PublishPanel component (packages/wp-story-editor/src/components/documentPane/publish/publish.js) below the poster image / publisher logo setting

Summary

Adds revisions link / icon to publish panel

Screen Shot 2022-09-08 at 6 34 32 AM

Relevant Technical Choices

To-do

User-facing changes

Testing Instructions

  • This is a non-user-facing change and requires no QA

This PR can be tested by following these steps:

  1. Enable improved autosaves experiment
  2. Create a new story
  3. Publish story
  4. There should now be a link to the revisions page in the Document tab
  5. Verify the link works as expected
  6. Make changes to story & save changes
  7. Verify the revision count increases and the link points to the revisions page for the latest revision

Note: improvements to the revisions page itself are not part of this ticket.

Reviews

Does this PR have a security-related impact?

No

Does this PR change what data or activity we track or use?

No

Does this PR have a legal-related impact?

No

Checklist

  • This PR addresses an existing issue and I have linked this PR to it in ZenHub
  • I have tested this code to the best of my abilities
  • I have verified accessibility to the best of my abilities (docs)
  • I have verified i18n and l10n (translation, right-to-left layout) to the best of my abilities
  • This code is covered by automated tests (unit, integration, and/or e2e) to verify it works as intended (docs)
  • I have added documentation where necessary
  • I have added a matching Type: XYZ label to the PR

Fixes #85

@timarney timarney self-assigned this Aug 31, 2022
@timarney timarney added Group: WordPress Changes related to WordPress or Gutenberg integration Group: Workspace A group encompassing all of workspace – everything inside the canvas and sidebar Pod: WP labels Aug 31, 2022
@swissspidy
Copy link
Collaborator

For reference, if it helps, here's how WP gets the necessary data to display:

https://github.com/WordPress/gutenberg/blob/7b519e482ca62a186c29d2c00e41c5947712f44f/packages/editor/src/store/selectors.js#L225-L251
https://github.com/WordPress/gutenberg/blob/33d84b036592a5bf31af05b7710f3b2b14163dc4/packages/editor/src/components/post-last-revision/index.js
https://github.com/WordPress/wordpress-develop/blob/69b9ecc54e2b2e1724f10d17dfd681aa8aee8810/src/wp-includes/rest-api/endpoints/class-wp-rest-posts-controller.php#L2066-L2081

Since we're only using this in wp-story-editor right now, getting this information from the REST API is probably a bit overkill. Also, don't want to assemble the wp-admin/revision.php link on the client side when we can do it in PHP.
So I think we can just update \Google\Web_Stories\Admin\Editor::get_editor_settings() and pass the revision count and revision link there.

@swissspidy swissspidy added the Type: Enhancement New feature or improvement of an existing feature label Aug 31, 2022
@swissspidy
Copy link
Collaborator

Actually, just realized that in Gutenberg the revision count is updated automatically when saving:

Screen.Recording.2022-08-31.at.17.32.46.mov

For this we'd need to pass the revision count & link via the REST API.

I guess we can do that too.

@swissspidy swissspidy mentioned this pull request Aug 31, 2022
8 tasks
@timarney
Copy link
Contributor Author

timarney commented Sep 2, 2022

Added the code to Editor.php for now to move this along a bit. Will look to move to a rest call.

@timarney
Copy link
Contributor Author

timarney commented Sep 7, 2022

For this we'd need to pass the revision count & link via the REST API.

@swissspidy is there an existing REST controller I should add a route to ? Or would this be a new controller / route?

@swissspidy
Copy link
Collaborator

We have Story_Controller with lots of precedence with custom additional fields. We can add these fields there, no need for a new route.

@timarney
Copy link
Contributor Author

timarney commented Sep 8, 2022

Code has been moved to the Story_Controller pulling data in via the story.

@timarney timarney marked this pull request as ready for review September 8, 2022 10:42
@googleforcreators-bot
Copy link
Collaborator

googleforcreators-bot commented Sep 8, 2022

Plugin builds for 77ce1ab are ready 🛎️!

@github-actions
Copy link
Contributor

github-actions bot commented Sep 8, 2022

Size Change: +845 B (0%)

Total Size: 2.68 MB

Filename Size Change
assets/js/wp-story-editor.js 1.35 MB +845 B (0%)
ℹ️ View Unchanged
Filename Size
assets/css/carousel-view-rtl.css 702 B
assets/css/carousel-view.css 701 B
assets/css/web-stories-block-rtl.css 4.52 kB
assets/css/web-stories-block.css 4.56 kB
assets/css/web-stories-embed-rtl.css 318 B
assets/css/web-stories-embed.css 317 B
assets/css/web-stories-list-styles-rtl.css 2.36 kB
assets/css/web-stories-list-styles.css 2.39 kB
assets/css/web-stories-theme-style-twentyeleven-rtl.css 102 B
assets/css/web-stories-theme-style-twentyeleven.css 102 B
assets/css/web-stories-theme-style-twentyfifteen-rtl.css 251 B
assets/css/web-stories-theme-style-twentyfifteen.css 251 B
assets/css/web-stories-theme-style-twentyfourteen-rtl.css 287 B
assets/css/web-stories-theme-style-twentyfourteen.css 287 B
assets/css/web-stories-theme-style-twentyseventeen-rtl.css 288 B
assets/css/web-stories-theme-style-twentyseventeen.css 288 B
assets/css/web-stories-theme-style-twentysixteen-rtl.css 224 B
assets/css/web-stories-theme-style-twentysixteen.css 224 B
assets/css/web-stories-theme-style-twentyten-rtl.css 143 B
assets/css/web-stories-theme-style-twentyten.css 143 B
assets/css/web-stories-theme-style-twentytwelve-rtl.css 256 B
assets/css/web-stories-theme-style-twentytwelve.css 256 B
assets/css/web-stories-theme-style-twentytwenty-rtl.css 86 B
assets/css/web-stories-theme-style-twentytwenty.css 86 B
assets/css/web-stories-theme-style-twentytwentyone-rtl.css 326 B
assets/css/web-stories-theme-style-twentytwentyone.css 326 B
assets/css/web-stories-widget-rtl.css 482 B
assets/css/web-stories-widget.css 482 B
assets/css/wp-dashboard-rtl.css 657 B
assets/css/wp-dashboard.css 659 B
assets/css/wp-story-editor-rtl.css 737 B
assets/css/wp-story-editor.css 738 B
assets/js/1814.js 7.45 kB
assets/js/208.js 67.7 kB
assets/js/2090.js 84.6 kB
assets/js/4319.js 34.9 kB
assets/js/4422.js 49.3 kB
assets/js/5509.js 100 kB
assets/js/7903.js 5.82 kB
assets/js/8107.js 92.2 kB
assets/js/9796.js 3.71 kB
assets/js/carousel-view.js 3.41 kB
assets/js/chunk-colorthief.js 2.64 kB
assets/js/chunk-ffmpeg.js 5.89 kB
assets/js/chunk-focus-visible.js 1.01 kB
assets/js/chunk-getStoryMarkup.js 12.3 kB
assets/js/chunk-html-to-image.js 4.5 kB
assets/js/chunk-opentype.js 96 B
assets/js/chunk-react-calendar.js 12.5 kB
assets/js/chunk-react-color.js 44.3 kB
assets/js/chunk-web-animations-js.js 14.6 kB
assets/js/chunk-web-stories-template-0-metaData.js 545 B
assets/js/chunk-web-stories-template-0.js 11.4 kB
assets/js/chunk-web-stories-template-1-metaData.js 539 B
assets/js/chunk-web-stories-template-1.js 9.61 kB
assets/js/chunk-web-stories-template-10-metaData.js 533 B
assets/js/chunk-web-stories-template-10.js 7.37 kB
assets/js/chunk-web-stories-template-11-metaData.js 539 B
assets/js/chunk-web-stories-template-11.js 9.09 kB
assets/js/chunk-web-stories-template-12-metaData.js 497 B
assets/js/chunk-web-stories-template-12.js 9.7 kB
assets/js/chunk-web-stories-template-13-metaData.js 525 B
assets/js/chunk-web-stories-template-13.js 7.4 kB
assets/js/chunk-web-stories-template-14-metaData.js 582 B
assets/js/chunk-web-stories-template-14.js 7.37 kB
assets/js/chunk-web-stories-template-15-metaData.js 544 B
assets/js/chunk-web-stories-template-15.js 9 kB
assets/js/chunk-web-stories-template-16-metaData.js 588 B
assets/js/chunk-web-stories-template-16.js 10.9 kB
assets/js/chunk-web-stories-template-17-metaData.js 539 B
assets/js/chunk-web-stories-template-17.js 9.2 kB
assets/js/chunk-web-stories-template-18-metaData.js 585 B
assets/js/chunk-web-stories-template-18.js 9.91 kB
assets/js/chunk-web-stories-template-19-metaData.js 501 B
assets/js/chunk-web-stories-template-19.js 10.8 kB
assets/js/chunk-web-stories-template-2-metaData.js 586 B
assets/js/chunk-web-stories-template-2.js 9.3 kB
assets/js/chunk-web-stories-template-20-metaData.js 548 B
assets/js/chunk-web-stories-template-20.js 9.01 kB
assets/js/chunk-web-stories-template-21-metaData.js 535 B
assets/js/chunk-web-stories-template-21.js 9.85 kB
assets/js/chunk-web-stories-template-22-metaData.js 525 B
assets/js/chunk-web-stories-template-22.js 7.83 kB
assets/js/chunk-web-stories-template-23-metaData.js 605 B
assets/js/chunk-web-stories-template-23.js 7.48 kB
assets/js/chunk-web-stories-template-24-metaData.js 516 B
assets/js/chunk-web-stories-template-24.js 11.7 kB
assets/js/chunk-web-stories-template-25-metaData.js 544 B
assets/js/chunk-web-stories-template-25.js 7.06 kB
assets/js/chunk-web-stories-template-26-metaData.js 601 B
assets/js/chunk-web-stories-template-26.js 7.27 kB
assets/js/chunk-web-stories-template-27-metaData.js 542 B
assets/js/chunk-web-stories-template-27.js 7.82 kB
assets/js/chunk-web-stories-template-28-metaData.js 532 B
assets/js/chunk-web-stories-template-28.js 9.07 kB
assets/js/chunk-web-stories-template-29-metaData.js 562 B
assets/js/chunk-web-stories-template-29.js 9.25 kB
assets/js/chunk-web-stories-template-3-metaData.js 539 B
assets/js/chunk-web-stories-template-3.js 8.42 kB
assets/js/chunk-web-stories-template-30-metaData.js 576 B
assets/js/chunk-web-stories-template-30.js 7.89 kB
assets/js/chunk-web-stories-template-31-metaData.js 502 B
assets/js/chunk-web-stories-template-31.js 10.3 kB
assets/js/chunk-web-stories-template-32-metaData.js 552 B
assets/js/chunk-web-stories-template-32.js 13.3 kB
assets/js/chunk-web-stories-template-33-metaData.js 492 B
assets/js/chunk-web-stories-template-33.js 9.07 kB
assets/js/chunk-web-stories-template-34-metaData.js 571 B
assets/js/chunk-web-stories-template-34.js 7.58 kB
assets/js/chunk-web-stories-template-35-metaData.js 566 B
assets/js/chunk-web-stories-template-35.js 8.91 kB
assets/js/chunk-web-stories-template-36-metaData.js 577 B
assets/js/chunk-web-stories-template-36.js 12.7 kB
assets/js/chunk-web-stories-template-37-metaData.js 528 B
assets/js/chunk-web-stories-template-37.js 6.71 kB
assets/js/chunk-web-stories-template-38-metaData.js 572 B
assets/js/chunk-web-stories-template-38.js 7.94 kB
assets/js/chunk-web-stories-template-39-metaData.js 588 B
assets/js/chunk-web-stories-template-39.js 8.08 kB
assets/js/chunk-web-stories-template-4-metaData.js 564 B
assets/js/chunk-web-stories-template-4.js 12.7 kB
assets/js/chunk-web-stories-template-40-metaData.js 557 B
assets/js/chunk-web-stories-template-40.js 10.2 kB
assets/js/chunk-web-stories-template-41-metaData.js 572 B
assets/js/chunk-web-stories-template-41.js 7.75 kB
assets/js/chunk-web-stories-template-42-metaData.js 521 B
assets/js/chunk-web-stories-template-42.js 7 kB
assets/js/chunk-web-stories-template-43-metaData.js 557 B
assets/js/chunk-web-stories-template-43.js 8.76 kB
assets/js/chunk-web-stories-template-44-metaData.js 584 B
assets/js/chunk-web-stories-template-44.js 11.1 kB
assets/js/chunk-web-stories-template-45-metaData.js 565 B
assets/js/chunk-web-stories-template-45.js 7.52 kB
assets/js/chunk-web-stories-template-46-metaData.js 531 B
assets/js/chunk-web-stories-template-46.js 5.22 kB
assets/js/chunk-web-stories-template-47-metaData.js 592 B
assets/js/chunk-web-stories-template-47.js 9.42 kB
assets/js/chunk-web-stories-template-48-metaData.js 555 B
assets/js/chunk-web-stories-template-48.js 9.09 kB
assets/js/chunk-web-stories-template-49-metaData.js 518 B
assets/js/chunk-web-stories-template-49.js 9.69 kB
assets/js/chunk-web-stories-template-5-metaData.js 556 B
assets/js/chunk-web-stories-template-5.js 9.94 kB
assets/js/chunk-web-stories-template-50-metaData.js 503 B
assets/js/chunk-web-stories-template-50.js 9.15 kB
assets/js/chunk-web-stories-template-51-metaData.js 526 B
assets/js/chunk-web-stories-template-51.js 10.4 kB
assets/js/chunk-web-stories-template-52-metaData.js 602 B
assets/js/chunk-web-stories-template-52.js 10.4 kB
assets/js/chunk-web-stories-template-53-metaData.js 553 B
assets/js/chunk-web-stories-template-53.js 5.78 kB
assets/js/chunk-web-stories-template-54-metaData.js 547 B
assets/js/chunk-web-stories-template-54.js 7.67 kB
assets/js/chunk-web-stories-template-55-metaData.js 573 B
assets/js/chunk-web-stories-template-55.js 7.13 kB
assets/js/chunk-web-stories-template-56-metaData.js 542 B
assets/js/chunk-web-stories-template-56.js 9.87 kB
assets/js/chunk-web-stories-template-57-metaData.js 527 B
assets/js/chunk-web-stories-template-57.js 14.9 kB
assets/js/chunk-web-stories-template-58-metaData.js 555 B
assets/js/chunk-web-stories-template-58.js 5.74 kB
assets/js/chunk-web-stories-template-59-metaData.js 590 B
assets/js/chunk-web-stories-template-59.js 8.96 kB
assets/js/chunk-web-stories-template-6-metaData.js 569 B
assets/js/chunk-web-stories-template-6.js 7.07 kB
assets/js/chunk-web-stories-template-60-metaData.js 510 B
assets/js/chunk-web-stories-template-60.js 9.51 kB
assets/js/chunk-web-stories-template-7-metaData.js 569 B
assets/js/chunk-web-stories-template-7.js 7.46 kB
assets/js/chunk-web-stories-template-8-metaData.js 569 B
assets/js/chunk-web-stories-template-8.js 8.93 kB
assets/js/chunk-web-stories-template-9-metaData.js 581 B
assets/js/chunk-web-stories-template-9.js 8.46 kB
assets/js/chunk-web-stories-templates.js 443 B
assets/js/chunk-web-stories-textset-0.js 5.06 kB
assets/js/chunk-web-stories-textset-1.js 6.65 kB
assets/js/chunk-web-stories-textset-2.js 7.65 kB
assets/js/chunk-web-stories-textset-3.js 15.1 kB
assets/js/chunk-web-stories-textset-4.js 4.15 kB
assets/js/chunk-web-stories-textset-5.js 5.47 kB
assets/js/chunk-web-stories-textset-6.js 5.28 kB
assets/js/chunk-web-stories-textset-7.js 10.2 kB
assets/js/generateBlurhash.worker.worker.js 1.1 kB
assets/js/imgareaselect.js 3.77 kB
assets/js/lightbox.js 550 B
assets/js/tinymce-button.js 2.84 kB
assets/js/web-stories-activation-notice.js 25.4 kB
assets/js/web-stories-block.js 18 kB
assets/js/web-stories-embed.js 20 B
assets/js/web-stories-widget.js 587 B
assets/js/wp-dashboard.js 64.1 kB

compressed-size-action

@spacedmonkey spacedmonkey mentioned this pull request Sep 8, 2022
8 tasks
@spacedmonkey
Copy link
Contributor

Spent a bit of time on this PR. On the face of it, I don't like the new PHP code added. Adding these new fields is not nessary as this data is already in the REST API in another format. It is currently hidden. So I put together another PR on top of this one that get that using existing fields. It is a WIP, but if you like the idea, I recommend merging into this branch and continuing to work on it. See #12264

It is worth noting that this data is in the links field. See this screenshot.

Screenshot 2022-09-08 at 16 56 22

@timarney timarney dismissed spacedmonkey’s stale review September 9, 2022 10:29

updated to use sprintf and _n

<Link
rel="noopener noreferrer"
target="_blank"
href={addQueryArgs(revisionLink, { revision: revisionId })}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may want to have a help function to get a revision link from config. Makin this a little more compatible with other cmses. Thoughts @swissspidy

Copy link
Collaborator

@swissspidy swissspidy Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The link cannot come from the config because it‘s dynamic. It should come from the REST API, which is why I‘ve originally suggested adding a new REST field to the API, so that we don‘t have to create the link here ourselves.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why can't it come from a config? How about a function like this.

getRevisionLink( revisionId )

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The final revision link is revision.php?revision=<id> where <id> changes every time you save the story in the editor.

Anyway, the current way is OK for now since this is in the wp-story-editor package, so other CMSes are not relevant here.

@spacedmonkey
Copy link
Contributor

I would love to see some kind of testing here. A unit test or karma test would be great.

Also, I believe now, we will need to update fixtures, so include the new fields.

…ublish.js

Co-authored-by: Jonny Harris <spacedmonkey@users.noreply.github.com>
@spacedmonkey

This comment was marked as duplicate.

@spacedmonkey
Copy link
Contributor

I wonder if we should up the number of revisions from 10. See

/**
* Force WordPress to only keep 10 revisions for the web stories post type.
*
* @since 1.16.0
*
* @param int $num Number of revisions to store.
* @return int Number of revisions to store.
*/
public function revisions_to_keep( $num ): int {
$num = (int) $num;
return ( $num >= 0 && $num < 10 ) ? $num : 10;
}

Copy link
Collaborator

@swissspidy swissspidy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just two minor nits otherwise looks good

@swissspidy swissspidy merged commit 09325a7 into main Sep 13, 2022
@swissspidy swissspidy deleted the fix/85-link-to-revisions branch September 13, 2022 03:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Group: WordPress Changes related to WordPress or Gutenberg integration Group: Workspace A group encompassing all of workspace – everything inside the canvas and sidebar Type: Enhancement New feature or improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor: Add link to revisions
4 participants