#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: |
Attachments (54)
Change History (134)
#5
@
12 years ago
- 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
#6
@
12 years ago
24425.draft.3.diff looks good, loads immediately even with many revisions. happy to test when ready.
#7
@
12 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 buttonnext/previous buttonsclicked revision not selected on initial loadcompare two mode- tick marks missing, simple tick marks might help visually (repeating background css?)
tooltipsrevision meta infoRTL mode (current revision should be in rightmost position when using RTL language)
newer code thats missing:
mouse dragging code 24425.5.diff- pagination support, see #24056 - not sure we need this?
remaining:
Compare two mode in RTL
#10
@
11 years ago
- 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
@
11 years ago
as per ocean90: replace $( '.wp-slider.ui-slider' ).mousemove() with this.$el.mousemove(), same for $( '.wp-slider.ui-slider' ).hover()
#11
@
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 oneseems 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
#12
@
11 years ago
- Severity changed from normal to blocker
Calling this one a blocker until it's RC-able.
#14
@
11 years ago
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.
#16
@
11 years ago
nice fixes, noticed that issue as well. should have other items addressed soon. thanks!
#17
@
11 years ago
- 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
#22
@
11 years ago
- correct tooltip placement in compare two mode
- enable two handled mode in RTL
- fixes issue with routes not loading correctly
- other minor bug fixes
#23
@
11 years ago
- 24425.draft.37.diff +
- Simplifies CSS code for tooltips
- Removes
jquery-ui-tooltip
as dependency - Adds
(no title)
if no title exists, props nacin
#27
@
11 years ago
- 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?
- 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.
- 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
@
11 years ago
- adds simple tickmarks behind slider
- includes bugfix lines from ocean90
next: make tooltips show only when near tickmark spot
#30
@
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.
#42
@
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
@
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
#46
@
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.
#50
@
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
#53
@
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
@
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.
#56
@
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:
↓ 75
@
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.
#59
@
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
@
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.
#68
@
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?
#74
follow-up:
↓ 76
@
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
@
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...
#76
in reply to:
↑ 74
@
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.
track mouse when dragging handles, snap to step when stopping