WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#24425 closed task (blessed) (fixed)

Revisions cleanup

Reported by: markjaquith Owned by:
Milestone: 3.6 Priority: high
Severity: blocker Version: 3.6
Component: Revisions Keywords:
Focuses: Cc:

Description

I'm looking over Revisions with Nacin and Koop. We think it needs a general cleanup, on the JS side, and for the Ajax endpoint. Tracking that on this ticket. This might run into some other tickets, but I didn't want to hijack them:

#24056
#24346
#24388

Attachments (54)

24425.diff (13.9 KB) - added by nacin 6 years ago.
24425.2.diff (14.8 KB) - added by nacin 6 years ago.
24425.3.diff (16.4 KB) - added by nacin 6 years ago.
24425.4.diff (16.3 KB) - added by nacin 6 years ago.
24425.5.diff (1.4 KB) - added by adamsilverstein 6 years ago.
track mouse when dragging handles, snap to step when stopping
24425.draft.diff (31.9 KB) - added by koopersmith 6 years ago.
24425.draft.2.diff (49.7 KB) - added by koopersmith 6 years ago.
24425.draft.3.diff (52.3 KB) - added by koopersmith 6 years ago.
24425.draft.4.diff (52.3 KB) - added by markjaquith 6 years ago.
Make sure fetch() doesn't remove models from the diffs collection
24425.draft.5.diff (55.2 KB) - added by markjaquith 6 years ago.
Some chunked loading.
24425.draft.6.diff (54.9 KB) - added by markjaquith 6 years ago.
24425.draft.7.diff (51.5 KB) - added by adamsilverstein 6 years ago.
set slider handle to intiially selected/passed revision
24425.draft.8.diff (55.1 KB) - added by adamsilverstein 6 years ago.
improved find the initially selected revision
24425.draft.9.diff (54.1 KB) - added by adamsilverstein 6 years ago.
adds previous/next buttons and functionality back
24425.draft.10.diff (57.7 KB) - added by adamsilverstein 6 years ago.
includes missing file
24425.draft.11.diff (60.0 KB) - added by adamsilverstein 6 years ago.
adds more features back, author meta
24425.draft.12.diff (60.2 KB) - added by adamsilverstein 6 years ago.
restore button functionality
24425.draft.14.diff (60.5 KB) - added by adamsilverstein 6 years ago.
remove nested object template kludge
24425.draft.15.diff (61.1 KB) - added by adamsilverstein 6 years ago.
adds back mouse tracking for handle while dragging
24425.draft.16.diff (61.8 KB) - added by adamsilverstein 6 years ago.
fixes extraneous ajax calls when using next/previous
24425.draft.17.diff (2.7 KB) - added by adamsilverstein 6 years ago.
completes Rtl functionality, diff against current trunk
24425.title-table-cols.patch (1.0 KB) - added by ocean90 6 years ago.
24425.draft.17b.diff (5.9 KB) - added by ocean90 6 years ago.
24425.draft.18.diff (16.6 KB) - added by adamsilverstein 6 years ago.
adds compare two mode, history tracking
24425.draft.19.diff (16.8 KB) - added by adamsilverstein 6 years ago.
hide next/previous buttons in compare two mode
24425.draft.20.diff (17.2 KB) - added by adamsilverstein 6 years ago.
after linting and cleanup
24425.draft.21.diff (22.3 KB) - added by adamsilverstein 6 years ago.
adds tooltip when hovering over slider bar
24425.draft.22.diff (22.3 KB) - added by adamsilverstein 6 years ago.
corrects tooltip placement
24425.draft.23.diff (22.6 KB) - added by adamsilverstein 6 years ago.
prop to attr/sanity check for tooltip calculated revision index, fixes console error
24425.draft.24.diff (22.8 KB) - added by adamsilverstein 6 years ago.
address chrome flickering z-index related issue
24425.draft.25.diff (22.7 KB) - added by adamsilverstein 6 years ago.
as per ocean90: replace $( '.wp-slider.ui-slider' ).mousemove() with this.$el.mousemove(), same for $( '.wp-slider.ui-slider' ).hover()
24425.draft.26.diff (23.2 KB) - added by ocean90 6 years ago.
Slider: Handle mouse events via Backbone events
24425.draft.27.diff (24.9 KB) - added by ocean90 6 years ago.
Moves Tooltip, Checkbox and Buttons to its own view
24425.draft.28.diff (24.7 KB) - added by ocean90 6 years ago.
Little cleanup
24425.draft.29.diff (26.7 KB) - added by adamsilverstein 6 years ago.
numerous fixes
24425.draft.30.diff (28.4 KB) - added by ocean90 6 years ago.
Removes debug cruft
24425.history-debounce.diff (2.2 KB) - added by markjaquith 6 years ago.
Debounces the URL updates
24425.draft.33.diff (3.0 KB) - added by adamsilverstein 6 years ago.
routing fixes, other minor changes
24425.draft.34.diff (8.6 KB) - added by ocean90 6 years ago.
Tooltip model and remove mouse dragging code
24425.draft.35.diff (7.5 KB) - added by ocean90 6 years ago.
CSS cleanup
24425.draft.36.diff (1.3 KB) - added by ocean90 6 years ago.
24425.draft.37.diff (5.3 KB) - added by adamsilverstein 6 years ago.
two handled rtl, bug fixes
24425.draft.38.diff (12.2 KB) - added by ocean90 6 years ago.
tooltip + no title
24425.draft.39.diff (4.4 KB) - added by adamsilverstein 6 years ago.
enables smooth handle dragging in two handle mode
24425.draft.40.diff (3.1 KB) - added by ocean90 6 years ago.
An idea for the revision title and l18n
24425.draft.41.diff (3.9 KB) - added by adamsilverstein 6 years ago.
adds simple tickmarks
24425.draft.42.diff (5.2 KB) - added by adamsilverstein 6 years ago.
only show tooltip when hovering over/near tickmark
24425.draft.43.diff (6.2 KB) - added by adamsilverstein 6 years ago.
stick tooltips to tickmarks
24425.draft.43b.diff (6.4 KB) - added by adamsilverstein 6 years ago.
alternate version, show tooltip when hovering, but stick to tickmark locations
24425.draft.44.diff (6.7 KB) - added by adamsilverstein 6 years ago.
center arrow under tooltip
24425.draft.13.diff (6.4 KB) - added by markjaquith 6 years ago.
24425.draft.31.diff (649 bytes) - added by adamsilverstein 6 years ago.
places arrow below tooltip to right in RTL mode
24425.draft.32.diff (1.1 KB) - added by adamsilverstein 6 years ago.
center arrow under tooltip (against trunk)
24425.slider.diff (5.6 KB) - added by markjaquith 6 years ago.

Download all attachments as: .zip

Change History (134)

@nacin
6 years ago

#1 @ocean90
6 years ago

  • Cc ocean90 added

#2 @adamsilverstein
6 years ago

  • Cc adamsilverstein@… added

@nacin
6 years ago

@nacin
6 years ago

@nacin
6 years ago

#3 @a.hoereth
6 years ago

  • Cc a.hoereth@… added

#4 @aaroncampbell
6 years ago

  • Cc aaroncampbell added

@adamsilverstein
6 years ago

track mouse when dragging handles, snap to step when stopping

#5 @adamsilverstein
6 years ago

24425.5.diff​

  • handle now follows mouse position when dragging, then snaps back to selected step when dragging stopped/mouseup
  • positioning tested in firefox, needs browser testing especially IE (just a guess)
  • removed step: 1, thats the default
  • added animate: true, this should enable clicking to slide the handles once tick code is removed
  • refresh may be too much, but does put handle(s) back where they belong
Last edited 6 years ago by adamsilverstein (previous) (diff)

#6 @adamsilverstein
6 years ago

24425.draft.3.diff looks good, loads immediately even with many revisions. happy to test when ready.

Last edited 6 years ago by adamsilverstein (previous) (diff)

#7 @adamsilverstein
6 years ago

thanks for all the work reworking the revisions code.

i've noticed a few missing items and am listing them here as a point of reference, hope this is helpful:

  • restore this revision button
  • next/previous buttons
  • clicked revision not selected on initial load
  • compare two mode
  • tick marks missing, simple tick marks might help visually (repeating background css?)
  • tooltips
  • revision meta info
  • RTL mode (current revision should be in rightmost position when using RTL language)

newer code thats missing:

remaining:

Compare two mode in RTL

Last edited 6 years ago by adamsilverstein (previous) (diff)

@markjaquith
6 years ago

Make sure fetch() doesn't remove models from the diffs collection

@markjaquith
6 years ago

Some chunked loading.

@adamsilverstein
6 years ago

set slider handle to intiially selected/passed revision

@adamsilverstein
6 years ago

improved find the initially selected revision

@adamsilverstein
6 years ago

adds previous/next buttons and functionality back

@adamsilverstein
6 years ago

includes missing file

@adamsilverstein
6 years ago

adds more features back, author meta

@adamsilverstein
6 years ago

restore button functionality

@adamsilverstein
6 years ago

remove nested object template kludge

@adamsilverstein
6 years ago

adds back mouse tracking for handle while dragging

@adamsilverstein
6 years ago

fixes extraneous ajax calls when using next/previous

#8 @markjaquith
6 years ago

In 24520:

Cleanup of the revisions screen, both on the PHP API side, and the JS.

  • Much simpler PHP API
  • Cleaner and more Backbone-y JS API
  • Consequently, does batch queries; this now scales up to hundreds of revisions

Currently missing, but much easier considering the cleaned up base:

  • Compare two mode
  • RTL

props koopersmith, nacin, adamsilverstein, ocean90. see #24425

@adamsilverstein
6 years ago

completes Rtl functionality, diff against current trunk

#9 @ocean90
6 years ago

In 24529:

Revisions UI: Fix table colgroup (Say what?) for post titles when they haven't been changed. see #24425.

@adamsilverstein
6 years ago

adds compare two mode, history tracking

#10 @adamsilverstein
6 years ago

24425.draft.18.diff​

  • reintroduces compare two revisions mode
  • adds backbone history (although this slows down interactions slightly, not sure its worth keeping) comment out line 'Backbone.history.start();' to see the difference, does enable browser back/forward buttons
  • compare two mode untested in Rtl mode, likely needs work

notes:

  • mouse dragging/tracking disabled in compare two mode
  • next/previous button interaction needs cleanup
  • next/previous buttons need to be hidden in compare two mode

@adamsilverstein
6 years ago

hide next/previous buttons in compare two mode

@adamsilverstein
6 years ago

after linting and cleanup

@adamsilverstein
6 years ago

adds tooltip when hovering over slider bar

@adamsilverstein
6 years ago

corrects tooltip placement

@adamsilverstein
6 years ago

prop to attr/sanity check for tooltip calculated revision index, fixes console error

@adamsilverstein
6 years ago

address chrome flickering z-index related issue

@adamsilverstein
6 years ago

as per ocean90: replace $( '.wp-slider.ui-slider' ).mousemove() with this.$el.mousemove(), same for $( '.wp-slider.ui-slider' ).hover()

@ocean90
6 years ago

Slider: Handle mouse events via Backbone events

@ocean90
6 years ago

Moves Tooltip, Checkbox and Buttons to its own view

@ocean90
6 years ago

Little cleanup

@adamsilverstein
6 years ago

numerous fixes

#11 @adamsilverstein
6 years ago

in 24425.draft.29.diff
Corrected:

  • Corrects tooltip is misplaced when in compare-2-mode
  • Removed bottom border from .revisions-controls div
  • Removed markup for tick marks
  • Remove markup for the spinner
  • Moved if ( this.model.revisions.length < 3 ) to checkbox view
  • remove $( '.compare-two-revisions' ).trigger( 'click' ); instead rely on route actions only (started)
  • Tooltip message moved into model for easier translation
  • Switched to new View for tooltip

Still Remaining:

  • RTL mode 2 handles
  • Improve getSliderPosition(), use more. (Idea: return an array with left and right, if single only left is filled with a value)
  • Restore the .pre class for the diff table
  • ignore revision id from URL, instead use #route/to/revision_id

Bugs

  • Compare-2-mode: Right handler on the first revisions, left on the last: Expected: See all lines in green color, since it's a diff from the first to the current one seems to work fine; 1st revision isn't blank (except when comparing from in single handle mode)
  • Router: If reloading a page it loses the handle positions (currently being messes with by passed revision id)
  • duplicate firing on next/previous
Last edited 6 years ago by adamsilverstein (previous) (diff)

@ocean90
6 years ago

Removes debug cruft

#12 @markjaquith
6 years ago

  • Severity changed from normal to blocker

Calling this one a blocker until it's RC-able.

#13 @ocean90
6 years ago

In 24549:

Revisions UI update:

  • Restore Compare two mode
  • Restore tooltips
  • RTL support
  • First pass for URL routing and history

props adamsilverstein, see #24425.

@markjaquith
6 years ago

Debounces the URL updates

#14 @markjaquith
6 years ago

24425.history-debounce.diff

The URL updating on mouse drag was massively slowing things down. This patch makes a debounced function (250ms currently), and then only calls that from a custom event that fires after the diff has rendered (diff rendering has its own throttling built in, so this will filter out diffId changes that happen so quickly that by the time they start to render, the diffId has already changed).

Also a little bit of cleanup, removing unnecessary variables, and moving the Router to get passed in a model, instead of the set-it-after thing that was happening.

Last edited 6 years ago by markjaquith (previous) (diff)

#15 @markjaquith
6 years ago

In 24556:

Debounce the Revisions UI URL updates as you scrub, for a massive speedup and a reduction in URL history detritus.

see #24425

#16 @adamsilverstein
6 years ago

nice fixes, noticed that issue as well. should have other items addressed soon. thanks!

#17 @adamsilverstein
6 years ago

24425.draft.33.diff​

  • change variable name from sliderHovering to showTooltip
  • consolidate handle routing with nifty code from ocean90: this.model.set( { compareTwoMode: '2' === handles } );
  • corrects routing behavior, reload works correctly as well original link

@adamsilverstein
6 years ago

routing fixes, other minor changes

@ocean90
6 years ago

Tooltip model and remove mouse dragging code

#18 @markjaquith
6 years ago

In 24565:

Revisions improvements

  • Consolidated router code
  • Corrected routing behavior
  • Tooltip model
  • Removed mouse dragging code

props adamsilverstein, ocean90. see #24425.

@ocean90
6 years ago

CSS cleanup

#19 follow-up: @ocean90
6 years ago

In 24574:

Revisions: CSS cleanup, see #24425.

#20 in reply to: ↑ 19 @adamsilverstein
6 years ago

Replying to ocean90:

In 24574:

Revisions: CSS cleanup, see #24425.

nice cleanup, great to see all those lines removed!

#21 @ocean90
6 years ago

In 24575:

Revisions improvements

Revert mouse dragging code which was accidentally removed in [24565].

see #24425.

@adamsilverstein
6 years ago

two handled rtl, bug fixes

#22 @adamsilverstein
6 years ago

in 24425.draft.37.diff:

  • correct tooltip placement in compare two mode
  • enable two handled mode in RTL
  • fixes issue with routes not loading correctly
  • other minor bug fixes
Last edited 6 years ago by adamsilverstein (previous) (diff)

#23 @ocean90
6 years ago

24425.draft.38.diff

  • 24425.draft.37.diff +
  • Simplifies CSS code for tooltips
  • Removes jquery-ui-tooltip as dependency
  • Adds (no title) if no title exists, props nacin

@ocean90
6 years ago

tooltip + no title

#24 follow-up: @ocean90
6 years ago

In 24578:

Revisions improvements

  • Corrected routing behavior
  • Simplified CSS for tooltips
  • Compare two mode for RTL
  • Support for posts without titles

props adamsilverstein, ocean90. see #24425.

@adamsilverstein
6 years ago

enables smooth handle dragging in two handle mode

#25 in reply to: ↑ 24 @adamsilverstein
6 years ago

Replying to ocean90:

In 24578:

Revisions improvements

  • Corrected routing behavior
  • Simplified CSS for tooltips
  • Compare two mode for RTL
  • Support for posts without titles

props adamsilverstein, ocean90. see #24425.

nice alternate time zone tag team patching! think we are getting close to rc ready here.

#26 @ocean90
6 years ago

In 24579:

Revisions improvements

  • Enable smooth sliding in Compare two mode

props adamsilverstein. see #24425.

@ocean90
6 years ago

An idea for the revision title and l18n

#27 @markjaquith
6 years ago

  1. In Chrome, I'm getting some avatar flicker as I scrub, even though all the revisions have the same avatar. I'm guessing something related to us rendering too quickly? Any way we can recycle author avatar DOM elements so we're not creating them each time and waiting for Chrome to render?
  2. We're not doing any pre-loading in compare-two mode. Which I think is okay, given the multiplicative effect of compare two mode. But when you drag the scrubber really quickly, it creates a flurry of individual requests, while you'll only be seeing the last one, the one the scrubber stopped on. I suggest that we debounce this and consolidate the requests into one. That is, if you flick your finger and generate on-demand requests for 1:3, 1:4, 1:5, 1:6 instead of being 4 separate requests, they should be gathered into one request.
  3. When doing an on-demand request, we should fade/remove the current view and show some sort of loading indicator. Currently it displays the current info which means there is a scrubber/view mismatch.

#28 @adamsilverstein
6 years ago

24425.draft.41.diff​

  • adds simple tickmarks behind slider
  • includes bugfix lines from ocean90

next: make tooltips show only when near tickmark spot

@adamsilverstein
6 years ago

adds simple tickmarks

@adamsilverstein
6 years ago

only show tooltip when hovering over/near tickmark

@adamsilverstein
6 years ago

stick tooltips to tickmarks

@adamsilverstein
6 years ago

alternate version, show tooltip when hovering, but stick to tickmark locations

@adamsilverstein
6 years ago

center arrow under tooltip

@adamsilverstein
6 years ago

places arrow below tooltip to right in RTL mode

#29 @markjaquith
6 years ago

In 24595:

Tick marks are back for revisions

  • Simple tick marks
  • Tooltips snap to tick marks
  • Tooltips arrow reverses side for LTR/RTL
  • Fix for routing issue where just the hash is changed in the URL bar relating to compare two mode

see #24425. props adamsilverstein, ocean90.

@adamsilverstein
6 years ago

center arrow under tooltip (against trunk)

#30 @markjaquith
6 years ago

What I'd like for on-demand loading (say, in compare-two mode, or for fast scrubbers in single mode):

A way to call ensure (a flag?) that says "I can wait for this". If that flag is set, requests are queued and then a debouncer coalesces them and passes them to load all together. It'll have to be based on $.Deferred, because ensure has to return a promise. Since we don't have a "collecting" debouncer, I was thinking about just maintaining a queue object with keys of diff IDs and values of a deferred object (they can all point to the same one). Then we can just have a dummy debouncer that just serves as a triggering device. When it goes off, we grab the queued ones, clear the queue, add them to the request, then iterate their deferreds, setting them to resolve when the load deferred does.

#31 @koopersmith
6 years ago

In 24600:

Revisions: Remove redundant method, see #24425.

#32 @koopersmith
6 years ago

In 24601:

Revisions: Use toggleClass instead of a conditional. See #24425.

#33 @koopersmith
6 years ago

In 24602:

Revisions: Remove redundant tagNames. Props markjaquith, see #24425.

#34 @koopersmith
6 years ago

In 24603:

Revisions: A healthy dose of pinking shears. See #24425.

#35 @koopersmith
6 years ago

In 24604:

Revisions: Remove unused template. See #24425.

#36 @koopersmith
6 years ago

In 24605:

Revisions: Reduce points where IDs are cast in JS. See #24425.

#37 @koopersmith
6 years ago

In 24606:

Revisions: Simplify how URLs are updated. See #24425.

#38 @koopersmith
6 years ago

In 24607:

Revisions: Remove unnecessary router property. See #24425.

#39 @koopersmith
6 years ago

In 24608:

Revisions: Simplify router property name. See #24425.

#40 @koopersmith
6 years ago

In 24609:

Revisions: Move data management into the frame state. See #24425.

This helps to solidify the separation between data and UI. We track the diffId internally now, as the property itself was never referenced, and was always derived from the 'from' and 'to' models. This also sets us up for better request debouncing and diff management.

#41 @koopersmith
6 years ago

In 24611:

Revisions: Debounce fetching Diffs when revisions are updated.

Fixes navigation and casting bugs.

See #24425.

#42 @duck_
6 years ago

[24520] made the comparator for the Revisions collection to be the revision's ID. This is incorrect behaviour since autosaves are overwritten. This means that an autosave may have a lower ID than some revisions, but contain newer content.

Instead the comparator should be date based, e.g.

comparator: function( a, b ) {
    var a_ = new Date(a.get("date")),
        b_ = new Date(b.get("date"));
    var cmp = (a_ > b_) - (a_ < b_);
    if (cmp == 0 && a.id != b.id)
        cmp = a.id < b.id ? -1 : 1;                                                                                                                                    
    return cmp;
},

Issues with this quick hack are that it doesn't compare down to the second so falling back on the ID may not be enough in some circumstances, it should be tested for cross-browser equality of date parsing (the date property contains an "@" character), and it's not very clear. Maybe it would be better to have a unix timestamp in the model for comparision.

#43 @a.hoereth
6 years ago

1) "To" string empty when initially opening revision.php
2) "Restore This Revision" not disabled for right most post, restoring this does not change any post content because right most revision is always equal to post.

The "Restore This Revision" button is sometimes disabled when changing back to right most revision, see second half: http://www.screenr.com/jSgH

Last edited 6 years ago by a.hoereth (previous) (diff)

#44 @markjaquith
6 years ago

In 24613:

Move the loading of surrounding diffs functionality into the model.

  • wp.revisions.log() — temporary measure for logging based on wp.revisions.debug
  • Return promises from functions that sometimes bail but normally return promises.

See #24425.

#45 @markjaquith
6 years ago

In 24615:

Pass back dateUnix (a unix timestamp) for better sorting of revisions in Backbone.

Props duck_. See #24425.

#46 @adamsilverstein
6 years ago

interesting bug possibly related to load sequence, if you are hovering over the slider while loading/reloading the tooltip is visible to the left, with no data. http://cl.ly/Q8YJ

as soon as you move the mouse the issue goes away.

#47 @markjaquith
6 years ago

In 24622:

Don't call updateDiff() in the Revisions FrameState model.

  • Stuff needs to hook in, so firing it early results in a half-rendered screen.
  • updateDiff() now returns a promise.
  • Now, in the frame view's render(), it calls updateDiff(), relying on its promise.

See #24425.

#48 @markjaquith
6 years ago

In 24627:

Revisions: Update the compare mode before rendering subviews.

See #24425.

#49 @markjaquith
6 years ago

In 24629:

Revisions: Get rid of an unneccessary ID. IDs are a bad idea.

See #24425.

#50 @adamsilverstein
6 years ago

latest looks great, loading issues are resolved except tooltip still showing blank if mouse stationary over slider when loading. also, we still need to address item 3 above: http://core.trac.wordpress.org/ticket/24425#comment:27

#51 @markjaquith
6 years ago

In 24636:

Revisions: remove some redundant code.

  • Move some slider code into the slider view that was loitering in the checkmark view.
  • this.$(), not $().
  • Cache a selector.
  • toggleClass(), not addClass() with a ternary.
  • Remove two classes with a single removeClass().

See #24425.

#52 @markjaquith
6 years ago

In 24638:

Revisions: simpler hash URLs. Misc refactoring.

  • Single mode: #at/:to
  • Compare two mode: #from/:from/to/:to
  • Make use of _.isUndefined()

See #24425.

#53 @markjaquith
6 years ago

Done for the night. As adamsilverstein mentioned, we still need to hide the diff and do a loading indicator whenever an on-demand fetch from the server results in a delay.

#54 @duck_
6 years ago

[24627] caused the previous/next buttons to not be present on initial load. They appear again if you toggle the checkbox. The problem that the compareTwoMode attribute doesn't have a default value, so the call to get() returns undefined. toggleClass() will then add the comparing-two-revisions class since it was not passed a boolean.

Adding a default of false in the FrameState model fixed this for me. It looks like it should always start in single comparison mode, but I wasn't entirely sure.

#55 @markjaquith
6 years ago

In 24646:

Revisions: fixes for routing and initial setting of compareTwoMode

  • Fix routing handler logic (compareTwoMode was reversed).
  • Fix routing handler typo (both should use the same handler).
  • Set compareTwoMode to false, to start. props duck_.

See #24425.

#56 @ethitter
6 years ago

The "time ago" displayed when comparing revisions doesn't properly account for the user's chosen timezone offset. The revisions metabox on the edit screen does, however.

In my case, my timezone is set to Eastern, but the "x hours ago" text when comparing revisions includes the difference from GMT.

Comparison: http://share.ethitter.com/f/18/
Metabox: http://share.ethitter.com/f/19/

#57 follow-up: @markjaquith
6 years ago

24425.slider.diff refactors the slider such that the sizing isn't done in JS, but CSS, so there's no resize lag. Widths are zone with percentages, instead. Still needs some tweaking to get the alignment pixel-perfect, or at least only off by one, in some cases.

#58 @markjaquith
6 years ago

In 24658:

Revisions: Bunch of refactoring and code cleanup

  • Extracted a lot of model-y stuff from the view code.
  • Slider now has a proper model, with communication with other models.
  • Use of get( foo ) instead of findWhere({ id: foo }).
  • Properly set the from diff when routing single mode.
  • Bind prev() and next() to their model.
  • Tick marks are now percentage based, which means the slider resizes with the browser, without JS resize events.
  • When scrubbing, the position of the scrubber is considered a hover, so you can fall off the timeline while still scrubbing and the tooltips will persist.
  • Tooltips fade in and out.
  • Tooltips hang around for a grace period instead of immediately going away. More forgiving.
  • Unused code paths removed.
  • Got rid of a bunch of view-to-view communication.
  • Use _.times() instead of a for loop.
  • Removed premature Math.floor() and Math.ceil() calls that were making things not add up to 100%.

See #24425.

#59 @markjaquith
6 years ago

So, there that is. There are a few minor issues there. Not quite getting pixel-perfect alignment, especially with slider handles. Will have to investigate.

Still need loading indicator, and a fix for the bug ethitter spotted.

#60 @markjaquith
6 years ago

Also, when scrubbing the "from" handle in compare-two mode, need to set the hoverRevision based on the "from" handle, not the "to" handle.

#61 @markjaquith
6 years ago

In 24660:

Revisions: Use jQuery animation.

The CSS3 animations caused issues because the element was still there, and there is no standard way to know when a CSS3 animation is done.

See #24425.

#62 @markjaquith
6 years ago

In 24661:

Revisions: Immediately do a disabled button check on ready().

Solves the issue of Prev/Next buttons not being disabled on initial load, when routing to an extremity.

See #24425.

#63 @nacin
6 years ago

In 24663:

Compare GMT to GMT. see #24425.

#64 @markjaquith
6 years ago

In 24664:

Revisions: real URLs and preloading of the requested diff.

  • Real URLs are being used now, using pushState. ?revision={id} or ?from={from}&to={to}.
  • Drop the redundant action=edit from the URLs (this is the default).
  • The initial comparison is preloaded, whether a single revision or a compare-two situation.

See #24425.

#65 @markjaquith
6 years ago

In 24666:

Revisions: use reject(), not fail(), when rejecting a deferred.

See #24425.

#66 @markjaquith
6 years ago

In 24668:

Revisions: sort by date, not modified, to match revisions meta box.

See #24425.

#67 @markjaquith
6 years ago

In 24671:

Revisions: Cleanup, bug fixes, refactoring, polish.

  • Hide the tooltip initially.
  • Fix a bug with routing.
  • Further separate the Slider model and view, refactoring its code.
  • More reliance on events than direct calls between areas.
  • Smarter background diff loading (single mode). Loads the diffs closest to your position first.
  • Removed a bunch of manual templating and render() methods. Now relies more on the WP Backbone Views functionality.
  • include the requested id in ensure:load.
  • new trigger: ensure, for ensure() attempts, regardless of whether they are already loaded.
  • pass along a promise in both ensure and ensure:load.
  • in ensure, remove requests for diffs we aready have

See #24425.

#68 @markjaquith
6 years ago

What remaining issues are people seeing? The one thing I haven't been able to kill yet is the not-quite-pixel-perfect alignment of the sliders and tooltips. Anything else?

#69 @markjaquith
6 years ago

In 24673:

Revisions: Use all the vendor prefixes for box-sizing.

See #24425.

#70 @markjaquith
6 years ago

In 24675:

Revisions: Tweak the slider image and alignment.

See #24425.

#71 @markjaquith
6 years ago

In 24677:

Revisions: pixel perfect tooltips (as far as I can tell), and better checkbox alignment in Firefox.

See #24425.

#72 @markjaquith
6 years ago

In 24678:

Revisions: Have the tooltip follow the "from" scrubber handle when that is the one being moved, in compare-two mode.

See #24425.

#73 @markjaquith
6 years ago

In 24679:

Revisions: Code and whitespace cleanup.

See #24425.

#74 follow-up: @markjaquith
6 years ago

  • Resolution set to fixed
  • Status changed from new to closed

Okay, I'm calling it. I think we can move on to individual tickets for bugs at this point. Thanks to everyone involved for all their efforts!

#75 in reply to: ↑ 57 @adamsilverstein
6 years ago

Replying to markjaquith:

24425.slider.diff refactors the slider such that the sizing isn't done in JS, but CSS, so there's no resize lag. Widths are zone with percentages, instead. Still needs some tweaking to get the alignment pixel-perfect, or at least only off by one, in some cases.

much better this way...

Last edited 6 years ago by adamsilverstein (previous) (diff)

#76 in reply to: ↑ 74 @adamsilverstein
6 years ago

Replying to markjaquith:

Okay, I'm calling it. I think we can move on to individual tickets for bugs at this point. Thanks to everyone involved for all their efforts!

great final sprint work! will test and let you know if I catch anything.

#77 @markjaquith
6 years ago

In 24686:

Revisions: more graceful tooltips

  • Use hoverIntent to prevent inadvertent display and provide more forgiving exploration.
  • Nice interruptable fading animation.
  • Subtle CSS transition when moving the tooltip side-to-side.

Fixes #24734. See #24425.

#78 @markjaquith
6 years ago

In 24688:

Revisions: force a scroll bar so that everything doesn't jerk around as you scrub due to the scroll bar popping in and out.

See #24425.

#79 @markjaquith
6 years ago

In 24699:

Turn off debug mode in revisions.

See #24425.

This ticket was mentioned in IRC in #wordpress-dev by ericandrewlewis. View the logs.


6 years ago

Note: See TracTickets for help on using tickets.