Make WordPress Core

Opened 11 years ago

Closed 11 years ago

Last modified 10 years ago

#24425 closed task (blessed) (fixed)

Revisions cleanup

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

Download all attachments as: .zip

Change History (134)

@nacin
11 years ago

#1 @ocean90
11 years ago

  • Cc ocean90 added

#2 @adamsilverstein
11 years ago

  • Cc adamsilverstein@… added

@nacin
11 years ago

@nacin
11 years ago

@nacin
11 years ago

#3 @a.hoereth
11 years ago

  • Cc a.hoereth@… added

#4 @aaroncampbell
11 years ago

  • Cc aaroncampbell added

@adamsilverstein
11 years ago

track mouse when dragging handles, snap to step when stopping

#5 @adamsilverstein
11 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 11 years ago by adamsilverstein (previous) (diff)

#6 @adamsilverstein
11 years ago

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

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

#7 @adamsilverstein
11 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 Compare two mode in RTL
  • 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:

Version 5, edited 11 years ago by adamsilverstein (previous) (next) (diff)

@markjaquith
11 years ago

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

@markjaquith
11 years ago

Some chunked loading.

@adamsilverstein
11 years ago

set slider handle to intiially selected/passed revision

@adamsilverstein
11 years ago

improved find the initially selected revision

@adamsilverstein
11 years ago

adds previous/next buttons and functionality back

@adamsilverstein
11 years ago

includes missing file

@adamsilverstein
11 years ago

adds more features back, author meta

@adamsilverstein
11 years ago

restore button functionality

@adamsilverstein
11 years ago

remove nested object template kludge

@adamsilverstein
11 years ago

adds back mouse tracking for handle while dragging

@adamsilverstein
11 years ago

fixes extraneous ajax calls when using next/previous

#8 @markjaquith
11 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
11 years ago

completes Rtl functionality, diff against current trunk

#9 @ocean90
11 years ago

In 24529:

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

@adamsilverstein
11 years ago

adds compare two mode, history tracking

#10 @adamsilverstein
11 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
11 years ago

hide next/previous buttons in compare two mode

@adamsilverstein
11 years ago

after linting and cleanup

@adamsilverstein
11 years ago

adds tooltip when hovering over slider bar

@adamsilverstein
11 years ago

corrects tooltip placement

@adamsilverstein
11 years ago

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

@adamsilverstein
11 years ago

address chrome flickering z-index related issue

@adamsilverstein
11 years ago

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

@ocean90
11 years ago

Slider: Handle mouse events via Backbone events

@ocean90
11 years ago

Moves Tooltip, Checkbox and Buttons to its own view

@ocean90
11 years ago

Little cleanup

@adamsilverstein
11 years ago

numerous fixes

#11 @adamsilverstein
11 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 11 years ago by adamsilverstein (previous) (diff)

@ocean90
11 years ago

Removes debug cruft

#12 @markjaquith
11 years ago

  • Severity changed from normal to blocker

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

#13 @ocean90
11 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
11 years ago

Debounces the URL updates

#14 @markjaquith
11 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 11 years ago by markjaquith (previous) (diff)

#15 @markjaquith
11 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
11 years ago

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

#17 @adamsilverstein
11 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
11 years ago

routing fixes, other minor changes

@ocean90
11 years ago

Tooltip model and remove mouse dragging code

#18 @markjaquith
11 years ago

In 24565:

Revisions improvements

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

props adamsilverstein, ocean90. see #24425.

@ocean90
11 years ago

CSS cleanup

#19 follow-up: @ocean90
11 years ago

In 24574:

Revisions: CSS cleanup, see #24425.

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

Replying to ocean90:

In 24574:

Revisions: CSS cleanup, see #24425.

nice cleanup, great to see all those lines removed!

#21 @ocean90
11 years ago

In 24575:

Revisions improvements

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

see #24425.

@adamsilverstein
11 years ago

two handled rtl, bug fixes

#22 @adamsilverstein
11 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 11 years ago by adamsilverstein (previous) (diff)

#23 @ocean90
11 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
11 years ago

tooltip + no title

#24 follow-up: @ocean90
11 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
11 years ago

enables smooth handle dragging in two handle mode

#25 in reply to: ↑ 24 @adamsilverstein
11 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
11 years ago

In 24579:

Revisions improvements

  • Enable smooth sliding in Compare two mode

props adamsilverstein. see #24425.

@ocean90
11 years ago

An idea for the revision title and l18n

#27 @markjaquith
11 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
11 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
11 years ago

adds simple tickmarks

@adamsilverstein
11 years ago

only show tooltip when hovering over/near tickmark

@adamsilverstein
11 years ago

stick tooltips to tickmarks

@adamsilverstein
11 years ago

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

@adamsilverstein
11 years ago

center arrow under tooltip

@adamsilverstein
11 years ago

places arrow below tooltip to right in RTL mode

#29 @markjaquith
11 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
11 years ago

center arrow under tooltip (against trunk)

#30 @markjaquith
11 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
11 years ago

In 24600:

Revisions: Remove redundant method, see #24425.

#32 @koopersmith
11 years ago

In 24601:

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

#33 @koopersmith
11 years ago

In 24602:

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

#34 @koopersmith
11 years ago

In 24603:

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

#35 @koopersmith
11 years ago

In 24604:

Revisions: Remove unused template. See #24425.

#36 @koopersmith
11 years ago

In 24605:

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

#37 @koopersmith
11 years ago

In 24606:

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

#38 @koopersmith
11 years ago

In 24607:

Revisions: Remove unnecessary router property. See #24425.

#39 @koopersmith
11 years ago

In 24608:

Revisions: Simplify router property name. See #24425.

#40 @koopersmith
11 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
11 years ago

In 24611:

Revisions: Debounce fetching Diffs when revisions are updated.

Fixes navigation and casting bugs.

See #24425.

#42 @duck_
11 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
11 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 11 years ago by a.hoereth (previous) (diff)

#44 @markjaquith
11 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
11 years ago

In 24615:

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

Props duck_. See #24425.

#46 @adamsilverstein
11 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
11 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
11 years ago

In 24627:

Revisions: Update the compare mode before rendering subviews.

See #24425.

#49 @markjaquith
11 years ago

In 24629:

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

See #24425.

#50 @adamsilverstein
11 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
11 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
11 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
11 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_
11 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
11 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
11 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
11 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
11 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
11 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
11 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
11 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
11 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
11 years ago

In 24663:

Compare GMT to GMT. see #24425.

#64 @markjaquith
11 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
11 years ago

In 24666:

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

See #24425.

#66 @markjaquith
11 years ago

In 24668:

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

See #24425.

#67 @markjaquith
11 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
11 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
11 years ago

In 24673:

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

See #24425.

#70 @markjaquith
11 years ago

In 24675:

Revisions: Tweak the slider image and alignment.

See #24425.

#71 @markjaquith
11 years ago

In 24677:

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

See #24425.

#72 @markjaquith
11 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
11 years ago

In 24679:

Revisions: Code and whitespace cleanup.

See #24425.

#74 follow-up: @markjaquith
11 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
11 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 11 years ago by adamsilverstein (previous) (diff)

#76 in reply to: ↑ 74 @adamsilverstein
11 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
11 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
11 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
11 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.


10 years ago

Note: See TracTickets for help on using tickets.