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

Fix link back to editor from revisions screen #3511

Merged
merged 21 commits into from Dec 13, 2017

Conversation

Projects
None yet
4 participants
@adamsilverstein
Contributor

adamsilverstein commented Nov 16, 2017

Description

Fix link back to editor from revisions screen.

  • Only show the revisions panel if at least 2 revisions exist. The revisions screen is not designed to display a single revision and is broken in this state. This behavior matches the current core behavior.
  • Add a gutenberg query var to the revisions link. This helps identify the correct link to use for returning to the editor from the revisions screen.
  • Only reset the classic editor link - if the gutenberg query var is present, use the gurenberg link.

Fixes #2808

How Has This Been Tested?

In gutenberg:

  • Create post with only title.

  • Change title.

  • Go to revisions screen.

  • Click return to editor link
    Confirm: back in gutenberg

  • Create a post with classic editor

  • repeat previous steps
    Confirm: back to editor link goes back to classic editor.

Checklist:

  • My code is tested.
    Actually, we could probably add a test to confirm the revisions link only appears when there at least two revisions. I feel like we had this test before.
  • My code follows the WordPress code style.
  • My code follows has proper inline documentation.
@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 16, 2017

Codecov Report

Merging #3511 into master will decrease coverage by 3.12%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3511      +/-   ##
==========================================
- Coverage   37.74%   34.61%   -3.13%     
==========================================
  Files         279      261      -18     
  Lines        6743     6769      +26     
  Branches     1227     1231       +4     
==========================================
- Hits         2545     2343     -202     
- Misses       3536     3733     +197     
- Partials      662      693      +31
Impacted Files Coverage Δ
editor/components/post-last-revision/index.js 0% <ø> (ø) ⬆️
editor/components/post-last-revision/check.js 0% <0%> (ø) ⬆️
components/panel/color.js 0% <0%> (-100%) ⬇️
blocks/url-input/button.js 0% <0%> (-100%) ⬇️
components/panel/row.js 0% <0%> (-100%) ⬇️
editor/components/warning/index.js 0% <0%> (-100%) ⬇️
components/notice/index.js 0% <0%> (-100%) ⬇️
editor/components/document-outline/index.js 17.85% <0%> (-57.15%) ⬇️
blocks/autocompleters/index.js 0% <0%> (-42.11%) ⬇️
editor/components/post-featured-image/index.js 22.22% <0%> (-17.78%) ⬇️
... and 199 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fc758d...ec2f7d1. Read the comment docs.

codecov bot commented Nov 16, 2017

Codecov Report

Merging #3511 into master will decrease coverage by 3.12%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3511      +/-   ##
==========================================
- Coverage   37.74%   34.61%   -3.13%     
==========================================
  Files         279      261      -18     
  Lines        6743     6769      +26     
  Branches     1227     1231       +4     
==========================================
- Hits         2545     2343     -202     
- Misses       3536     3733     +197     
- Partials      662      693      +31
Impacted Files Coverage Δ
editor/components/post-last-revision/index.js 0% <ø> (ø) ⬆️
editor/components/post-last-revision/check.js 0% <0%> (ø) ⬆️
components/panel/color.js 0% <0%> (-100%) ⬇️
blocks/url-input/button.js 0% <0%> (-100%) ⬇️
components/panel/row.js 0% <0%> (-100%) ⬇️
editor/components/warning/index.js 0% <0%> (-100%) ⬇️
components/notice/index.js 0% <0%> (-100%) ⬇️
editor/components/document-outline/index.js 17.85% <0%> (-57.15%) ⬇️
blocks/autocompleters/index.js 0% <0%> (-42.11%) ⬇️
editor/components/post-featured-image/index.js 22.22% <0%> (-17.78%) ⬇️
... and 199 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2fc758d...ec2f7d1. Read the comment docs.

@adamsilverstein adamsilverstein requested review from youknowriad and aduth and removed request for aduth Nov 16, 2017

@aduth

aduth approved these changes Nov 16, 2017

Show outdated Hide outdated lib/register.php
Show outdated Hide outdated lib/register.php
@jacboy

This comment has been minimized.

Show comment
Hide comment
@jacboy

jacboy Dec 5, 2017

Would this solve my problem? How can I apply this to Gutenberg v 1.8.1?

jacboy commented Dec 5, 2017

Would this solve my problem? How can I apply this to Gutenberg v 1.8.1?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 5, 2017

Member

Another issue here is that restoring the revision still returns to the classic editor, even when navigating revisions from Gutenberg. I'm going to take a look.

@jacboy I expect it would resolve once merged to master, at which point you'd either need to wait for the next release or run from the repository using these instructions.

Member

aduth commented Dec 5, 2017

Another issue here is that restoring the revision still returns to the classic editor, even when navigating revisions from Gutenberg. I'm going to take a look.

@jacboy I expect it would resolve once merged to master, at which point you'd either need to wait for the next release or run from the repository using these instructions.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 5, 2017

Member

Rebased and pushed with suggested revisions, including a fix for the issue described in #3511 (comment).

@adamsilverstein Would you have availability to review these latest changes?

Member

aduth commented Dec 5, 2017

Rebased and pushed with suggested revisions, including a fix for the issue described in #3511 (comment).

@adamsilverstein Would you have availability to review these latest changes?

@jacboy

This comment has been minimized.

Show comment
Hide comment
@jacboy

jacboy Dec 5, 2017

@aduth Thanks for your answer. As I can't handle these steps to build the plugin, I kindly want to ask if there is a roadmap for the next release fixing this issue or a workaround on how to get the content back?

jacboy commented Dec 5, 2017

@aduth Thanks for your answer. As I can't handle these steps to build the plugin, I kindly want to ask if there is a roadmap for the next release fixing this issue or a workaround on how to get the content back?

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 5, 2017

Member

@jacboy Releases are intended to occur approximately every week, though with WordCamp US this past weekend, it may be until next week before the next release.

Member

aduth commented Dec 5, 2017

@jacboy Releases are intended to occur approximately every week, though with WordCamp US this past weekend, it may be until next week before the next release.

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Dec 8, 2017

Contributor

@aduth I'll review this today :)

Contributor

adamsilverstein commented Dec 8, 2017

@aduth I'll review this today :)

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Dec 8, 2017

Contributor

@aduth this tests well locally, going to think about how we can add some test coverage

Contributor

adamsilverstein commented Dec 8, 2017

@aduth this tests well locally, going to think about how we can add some test coverage

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Dec 8, 2017

Contributor

@aduth I added some phpunit tests to validate the filters we have applied.

I tried working on jest to test PostLastRevisionCheck which should only render if revisionsCount > 1 in this commit - adamsilverstein@a32807c. I got stuck trying to set up the test (I get Invariant Violation: Could not find "store" in either the context or props of "Connect(PostLastRevisionCheck)". Either wrap the root component in a <Provider>, or explicitly pass "store" as a prop to "Connect(PostLastRevisionCheck)". because I'm doing it wrong. Any help there appreciated or we can leave that test off.

Contributor

adamsilverstein commented Dec 8, 2017

@aduth I added some phpunit tests to validate the filters we have applied.

I tried working on jest to test PostLastRevisionCheck which should only render if revisionsCount > 1 in this commit - adamsilverstein@a32807c. I got stuck trying to set up the test (I get Invariant Violation: Could not find "store" in either the context or props of "Connect(PostLastRevisionCheck)". Either wrap the root component in a <Provider>, or explicitly pass "store" as a prop to "Connect(PostLastRevisionCheck)". because I'm doing it wrong. Any help there appreciated or we can leave that test off.

@aduth

This comment has been minimized.

Show comment
Hide comment
@aduth

aduth Dec 11, 2017

Member

@adamsilverstein It's because the default export of post-last-revision/index.js is the React-Redux connected component which is assuming to find a store object in context. So you'd either need to provide this context or, more preferably, test the unconnected component and pass in props you might expect to be populated by the connect.

You can see an example of this here:

export class AutosaveMonitor extends Component {

Where in addition to the default export, we also create a separate named export for the class itself, which is used in the tests:

import { AutosaveMonitor } from '../';

Member

aduth commented Dec 11, 2017

@adamsilverstein It's because the default export of post-last-revision/index.js is the React-Redux connected component which is assuming to find a store object in context. So you'd either need to provide this context or, more preferably, test the unconnected component and pass in props you might expect to be populated by the connect.

You can see an example of this here:

export class AutosaveMonitor extends Component {

Where in addition to the default export, we also create a separate named export for the class itself, which is used in the tests:

import { AutosaveMonitor } from '../';

@jacboy

This comment has been minimized.

Show comment
Hide comment
@jacboy

jacboy Dec 11, 2017

1.9.0 has been released, this is still not working. Issue is open so I guess this hasn't been implemented?

jacboy commented Dec 11, 2017

1.9.0 has been released, this is still not working. Issue is open so I guess this hasn't been implemented?

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Dec 12, 2017

Contributor

@aduth I added some tests by exporting LastRevision explicitly, not sure about the exact approach but it does validate the functionality.

Contributor

adamsilverstein commented Dec 12, 2017

@aduth I added some tests by exporting LastRevision explicitly, not sure about the exact approach but it does validate the functionality.

@aduth

aduth approved these changes Dec 12, 2017

Ok, I think I misunderstood how these components were set up in my previous recommendations. For shallow testing, we'd want to ensure we're targeting the specific component we're testing, in this case the PostLastRevisionCheck component. So this is the one which should have a named export (from check.js). I'm honestly not sure how the previous tests were working, but we shouldn't be reaching into inner component behaviors for shallow testing.

I pushed updated tests with resolved lint issues in 4c13d2c

@aduth aduth merged commit e69fc0a into master Dec 13, 2017

3 checks passed

codecov/project 40.6% (+2.46%) compared to d43ef8d
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@aduth aduth deleted the issues-2808 branch Dec 13, 2017

@adamsilverstein

This comment has been minimized.

Show comment
Hide comment
@adamsilverstein

adamsilverstein Dec 15, 2017

Contributor

Nice!

Contributor

adamsilverstein commented on 4c13d2c Dec 15, 2017

Nice!

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Jan 10, 2018

Contributor

Thanks for working on this.

Contributor

mtias commented Jan 10, 2018

Thanks for working on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment