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

Revisions #4: Add revisions list and revisions diff viewer #14927

Merged
merged 12 commits into from
Aug 21, 2017

Conversation

bperson
Copy link
Contributor

@bperson bperson commented Jun 8, 2017

Note: This builds on top of #14242, so it's not merge-able yet

4th chunk coming from my WIP PR to introduce post revisions in calypso: #13367, this is mostly UI code to introduce a list of revisions in the post editor sidebar, a diff viewer replacing the main editor with a diff view of the revision currently selected and some code to enable "loading" a revision in the editor.

testing

  • Tap on "x revisions" in the post editor sidebar (shown only on posts with multiple revisions)
  • It should automatically fetch the revisions for the post you're currently editing
  • On desktop, once the revisions are fetched, it should automatically display the latest revision in the diff viewer
  • On mobile, once the revisions are fetched, it should NOT automatically select a revision.
  • On mobile, selecting a revision should switch focus to the diff viewer, you can then reopen the revisions sidebar by tap-ing the "history" icon in the editor ground control.
  • Once you have a revision selected in the list, you can "load the revision in the editor", on mobile, this should dismiss the entire sidebar.

screenshots

desktop

mobile

@bperson bperson added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Jun 8, 2017
@matticbot matticbot added OSS Citizen [Size] XL Probably needs to be broken down into multiple smaller issues labels Jun 8, 2017
@bperson bperson force-pushed the add/revisions-list-diff-viewer branch from 27b769d to 4ca1184 Compare June 19, 2017 16:49
@bperson
Copy link
Contributor Author

bperson commented Jun 19, 2017

Now that #14943 is merged, CircleCI is green, it's also rebased on the latest master

@bperson bperson force-pushed the add/revisions-list-diff-viewer branch from 8e5f936 to 4d30ae6 Compare June 20, 2017 20:27
@bperson
Copy link
Contributor Author

bperson commented Jun 20, 2017

Updates

@folletto
Copy link
Contributor

cap-drop

I did a design review. Looks REALLY good so far. I have some minor notes:

  1. Text seem using a light gray. Switch "X ago by Name" to $gray-dark.
  2. Some blocks in the list have no subtitle (editor-revisions-list__changes). Why is that? Can we make sure it's always present?
  3. It has some odd elements inside on mobile (screenshot, also the author in the top screenshot above). Can we make sure they don't pop-in on mobile?

I'm sure we can iterate and improve this further, but I think it's already a huge improvement, so let's try to ship this. :)

@bperson
Copy link
Contributor Author

bperson commented Jun 21, 2017

Text seem using a light gray. Switch "X ago by Name" to $gray-dark.

Updated, should we keep the hover color coming from using Buttons though? (we still have the background color changing on hover right now).

Some blocks in the list have no subtitle (editor-revisions-list__changes). Why is that? Can we make sure it's always present?

AFAIK, this should only happen when there is no "word changes" detected between 2 revisions, for instance if you change a number or punctuation (which aren't counted as words by countWords).

I'm not sure whether it's worth it to introduce a "less-strict" count words for revisions (making it inconsistent with the word count at the bottom of the editor) though?

screen shot 2017-06-21 at 12 01 28

It has some odd elements inside on mobile (screenshot, also the author in the top screenshot above). Can we make sure they don't pop-in on mobile?

Missed it during the split from the WIP PR :/, it's fixed:

screen shot 2017-06-21 at 12 09 36

@folletto
Copy link
Contributor

Updated, should we keep the hover color coming from using Buttons though? (we still have the background color changing on hover right now).

Are we sure inheriting from buttons is a good idea?

Anyhow, the design is meant to be like the accordions styling:

screen shot 2017-06-21 at 12 38 55

Notice the hover turns the text blue. Sorry, I recognize it wasn't evident from the mockups.

AFAIK, this should only happen when there is no "word changes" detected between 2 revisions, for instance if you change a number or punctuation

Odd. I had it on the first revision too, which was fairly big. If it's "smaller" we could just say "Minor changes" (in $gray). Mostly because it breaks the rhythm to have some blocks smaller (for this kind of design, different designs might build on that as a feature).

@bperson
Copy link
Contributor Author

bperson commented Jun 21, 2017

Are we sure inheriting from buttons is a good idea?

probably not, considering how much we're "tweaking" it, I've updated it and the rest of the CSS to more closely match what can be found in an accordion:

2 nitpick questions:

  • with the accordion spacing, the subtitle (the changes summary) seems a bit close to the title ("x ago by who")? Maybe the "g" (from "ago") and the "d" (from "word") making it weird?
  • in the accordion, the subtitle is in $gray-text-min, should I also use this? (instead of $gray)

screen shot 2017-06-21 at 14 49 03

Odd. I had it on the first revision too, which was fairly big. If it's "smaller" we could just say "Minor changes" (in $gray). Mostly because it breaks the rhythm to have some blocks smaller (for this kind of design, different designs might build on that as a feature).

Oops, my bad, it will also occur on the first revisions, right now it's defaulting to no change but we can very easily change that to consider that every word is actually "added" from what essentially was a blank page before. Since I would like to keep this PR on UI code alone, I'll cut another PR to fix it.

I've also added "minor changes" for all cases where we don't have "word diff" to not break the rhythm:

screen shot 2017-06-21 at 14 48 21

@folletto
Copy link
Contributor

with the accordion spacing, the subtitle (the changes summary) seems a bit close to the title ("x ago by who")? Maybe the "g" (from "ago") and the "d" (from "word") making it weird?

Yes, good point. Try adding some to your discretion and let's review seeing the screenshots :)

in the accordion, the subtitle is in $gray-text-min, should I also use this?

Good point too, let's do that.

it will also occur on the first revisions, right now it's defaulting to no change but we can very easily change that to consider that every word is actually "added" from what essentially was a blank page before. Since I would like to keep this PR on UI code alone, I'll cut another PR to fix it.

Sounds good. It's valuable as sometimes the bulk is added immediately and sometimes it's jut a few words. :)

I've also added "minor changes" for all cases where we don't have "word diff":

Excellent. 👍

@bperson
Copy link
Contributor Author

bperson commented Jun 21, 2017

5px of padding:

screen shot 2017-06-21 at 15 13 22

Good point too, let's do that.

updated 👌

@folletto
Copy link
Contributor

Cool!
Design wise we're good 👍

@bperson
Copy link
Contributor Author

bperson commented Jun 21, 2017

Awesome!, thanks a lot for the review @folletto, I've created #15354 to fix the issue with the first revision change summary defaulting to no change.

It's time to find some code reviewers now ;)

@matticbot
Copy link
Contributor

@bperson This PR needs a rebase

@matticbot
Copy link
Contributor

@bperson This PR needs a rebase

@matticbot
Copy link
Contributor

@bperson This PR needs a rebase

@bperson bperson force-pushed the add/revisions-list-diff-viewer branch from 5122f83 to 0d1618b Compare July 4, 2017 04:25
@mcsf mcsf force-pushed the add/editor-nested-sidebar branch from bd82ebc to 8b1e3ee Compare August 10, 2017 07:44
@mcsf mcsf self-assigned this Aug 10, 2017
@matticbot
Copy link
Contributor

@bperson This PR needs a rebase

@mcsf mcsf force-pushed the add/revisions-list-diff-viewer branch from 1ecf66b to d532714 Compare August 12, 2017 11:23
@mcsf mcsf changed the base branch from add/editor-nested-sidebar to master August 12, 2017 11:27
},

loadRevision: function( revision ) {
this.toggleNestedSidebar( NESTED_SIDEBAR_NONE );
Copy link
Member

@jblz jblz Aug 12, 2017

Choose a reason for hiding this comment

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

this.toggleNestedSidebar does not exist since the rebase

Copy link
Member

Choose a reason for hiding this comment

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

Missed it in the rebase. Fixed.

@mcsf mcsf force-pushed the add/revisions-list-diff-viewer branch from 62657a8 to 6de7aa2 Compare August 12, 2017 12:45
@jblz
Copy link
Member

jblz commented Aug 12, 2017

@mcsf Here's a demo of the issue I showed you:
https://cloudup.com/c3--PP5Q2oS

If you start typing in the content portion of the editor from the start, the draft gets saved with the initial content, a post id gets placed in the URL bar, etc, but it looks like no revision exists until a subsequent save.

This seems like an editor bug that's outside this PR & playing with this just surfaced it. I'm documenting here, so we're sure to follow up with this in a separate issue since there's a possibility of data loss since the initial contents don't make it into the list of revisions.

@folletto
Copy link
Contributor

This seems like an editor bug that's outside this PR & playing with this just surfaced it. I'm documenting here, so we're sure to follow up with this in a separate issue since there's a possibility of data loss since the initial contents don't make it into the list of revisions.

Did you already open the new issue? Seems a rough edge that as you mention might have impact even outside the revisions discussion.

@jblz
Copy link
Member

jblz commented Aug 14, 2017

@folletto just did #17183. Thanks.

@mcsf mcsf force-pushed the add/revisions-list-diff-viewer branch from 6de7aa2 to ad4121b Compare August 21, 2017 15:57
Copy link
Contributor

@spen spen left a comment

Choose a reason for hiding this comment

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

Just added a few comments - nothing blocking though :)

<div className="editor-revisions-list__header">
<Button
className="editor-revisions-list__load-revision"
compact={ true }
Copy link
Contributor

Choose a reason for hiding this comment

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

We could lose ={ true }, since true would be the implied value of compact without a given value.

this.props.revisions.length > 0 &&
isWithinBreakpoint( '>660px' )
) {
this.props.selectRevision( this.props.revisions[ 0 ].id );
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we use _.first here?
We could call it in place or outside of the conditional to give us something like:

const firstRevision = first( revisions );
if (
  ...
  !! firstRevision &&
  ...
) {
  selectRevision( firstRevision.id );
}

Also, will there ever chance of .id being undefined?

Copy link
Member

Choose a reason for hiding this comment

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

Very subjective, but when I applied this suggestion I didn't really like it. :) Maybe because the usage of first is so local, or because of the const that gets introduced, requiring a !! cast, instead of just relying on length.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fair fair :P

<span className="editor-revisions-list__date">
<PostTime date={ this.props.revision.date } />
</span>
&nbsp;
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the span below could come up empty if isObject( this.props.revision.author ) is falsey.
In those cases would a nbsp cause some off-centring? Perhaps the nbsp would be better off inside of that conditional

Copy link
Member

Choose a reason for hiding this comment

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

Good spotting, but local testing doesn't show any centring issues. I'll leave as-is.

// NOTE: Make sure we scroll back to the top AND trigger a scroll
// event no matter the scroll position we're coming from.
// ( used to force-reset TinyMCE toolbar )
window.scrollTo( 0, 1 );
Copy link
Contributor

Choose a reason for hiding this comment

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

smart :)

<FeaturedImage
site={ site }
post={ this.state.post }
maxWidth={ 1462 } />
Copy link
Contributor

Choose a reason for hiding this comment

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

I see that this was existing - but should we generally use constants for values like this? 1462 looks arbitrary here - a constant could give it some meaning & context.

Copy link
Member

Choose a reason for hiding this comment

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

Indeed, this should be better explained. I traced it back to 10818-gh-calypso-pre-oss 😅 . As the saying goes, patches welcome :trollface: . (In any case, doesn't belong in this PR; not to mention that the way GitHub renders the diff, it'd be very hard to spot that change in here.)

@mcsf mcsf merged commit ede8666 into master Aug 21, 2017
@matticbot matticbot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Aug 21, 2017
@marekhrabe marekhrabe deleted the add/revisions-list-diff-viewer branch October 19, 2017 14:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OSS Citizen [Size] XL Probably needs to be broken down into multiple smaller issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants