WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months ago

Last modified 23 months ago

#23935 closed enhancement (fixed)

Revision UI cleanup / improvements / corrections

Reported by: adamsilverstein Owned by:
Milestone: 3.6 Priority: normal
Severity: normal Version: 3.6
Component: Revisions Keywords:
Focuses: Cc:

Description (last modified by ocean90)

The new revisions UI still needs some UI polish. Now that we have addressed code/functionality issues i thought i would open a ticket to note the outstanding visual issues not addressed elsewhere:

  • the tooltips need work: their placement varies by browser, also it would be ideal if the tooltip stayed open while a user dragged the handle - currently the tooltip only stays open and updates if you keep your mouse confined to the slider handle; the slider on the other hand works fine if you drag and keep dragging but don't stay in the handle with your mouse; the result is that if you move off the handle while dragging, the slider moves, but the tooltip disappears. [24174]
  • the weighted tick marks get off to the left of center when they get thicker because of the way they are implemented; should fix so they are more accurately centered under the slider handle; last tick mark missing in compare two mode? #24056
  • the slider handle graphics are currently encoded as base 64 in the css, not certain thats the best way to implement them #23899
  • the 'compare two mode' option placement in the upper right seems haphazard, its just floating out there [24119]
  • Also we lost the 'single column' option for the diff view - not sure if we still want it, but i liked it.
  • with just a few revisions the slider/next/previous seem disconnected; with over a certain number of revisions the slider will grow wider than the screen; #24056

Attachments (20)

combined-diff.png (51.3 KB) - added by bpetty 2 years ago.
23935.screen-options.patch (4.3 KB) - added by ocean90 2 years ago.
23935.compare-revisions-checkbox.patch (300 bytes) - added by jrbeilke 2 years ago.
23935.compare-revisions-checkbox.2.patch (586 bytes) - added by ocean90 2 years ago.
23935.checkbox-and-status.patch (1.8 KB) - added by jrbeilke 2 years ago.
23935.tooltips-checkbox-status.patch (3.3 KB) - added by jrbeilke 2 years ago.
combined patch for slider tooltips, compare checkbox, and loading status
23935.tooltips-checkbox-status.2.patch (3.4 KB) - added by jrbeilke 2 years ago.
updating my earlier patch to include a fix for the revision column widths bug
23935.tooltips-checkbox-status.3.patch (3.4 KB) - added by jrbeilke 2 years ago.
split css selectors to separate lines per the coding standards
23935.patch (19.7 KB) - added by ocean90 2 years ago.
23935-improved-tick-marks.diff (11.1 KB) - added by adamsilverstein 2 years ago.
restore-eater-egg.diff (447 bytes) - added by adamsilverstein 2 years ago.
improved-tick-marks.diff (11.1 KB) - added by adamsilverstein 2 years ago.
23935-improved-tick-marks.2.diff (11.1 KB) - added by adamsilverstein 2 years ago.
23935-improved-tick-marks.3.diff (11.1 KB) - added by adamsilverstein 2 years ago.
23935-improved-tick-marks.4.diff (11.6 KB) - added by adamsilverstein 2 years ago.
add arrow below tooltips
23935-improved-tick-marks.5.diff (12.1 KB) - added by adamsilverstein 2 years ago.
remove color for range
23935-improved-tick-marks.6.diff (12.5 KB) - added by adamsilverstein 2 years ago.
add color-classic.css
23935-improved-tick-marks.7.diff (13.0 KB) - added by ocean90 2 years ago.
23935-improved-tick-marks.8.diff (14.3 KB) - added by adamsilverstein 23 months ago.
fixes last tick mark scope of changes not loaded correctly, fixes slider width calculation
23935-improved-tick-marks.9.diff (14.3 KB) - added by adamsilverstein 23 months ago.
fix width calculation

Download all attachments as: .zip

Change History (53)

comment:1 @ocean902 years ago

  • Keywords needs-ui added
  • Milestone changed from Awaiting Review to 3.6
  • Version changed from 3.5.1 to trunk

@bpetty2 years ago

comment:2 @bpetty2 years ago

Maybe I'm the only one that thinks this, but it just seems like a terrible idea for the title and excerpt content to be prepended and appended (respectively) right inline with the actual post content in the diff. The diff currently makes you think the new title and excerpt was accidentally included right in the post content.

I understand that it's nice to be able to see those changes along with the post content, but I really think those need to be in their own sections if they are going to be included.

So currently it looks like this:


But it should probably look more like this:

Title:

old title => new title

Content:

first post => second post

Excerpt:

old excerpt => new excerpt

comment:3 @bpetty2 years ago

  • Cc bpetty added

I should note that it's actually even worse when you go from no title/excerpt to a new one since there isn't even a "removed" line for it, so it definitely looks like it was just added to the main post content.

comment:4 @bpetty2 years ago

Related: #23899

comment:5 @ocean902 years ago

Related: #24056

comment:6 @ocean902 years ago

bpetty's point is handled in #23903.

comment:7 @ocean902 years ago

  • Description modified (diff)

comment:8 @jrbeilke2 years ago

  • Cc jrbeilke@… added

Ran into an issue with the new Revisions UI where the post title is overlapping the compare two revisions checkbox:
http://i.imgur.com/nwCONxF.jpg

comment:9 @ocean902 years ago

The patch from jrbeilke moves the compare 2 checkbox to the diff header: http://cl.ly/ON9R

Still need a better place for the loading status. Any suggestions?

comment:10 @jrbeilke2 years ago

adamsilverstein suggested we could move the loading-status just below the slider, since the status and the slider are directly related: http://i.imgur.com/Tq0Lf3f.jpg

diff incoming

Last edited 2 years ago by jrbeilke (previous) (diff)

@jrbeilke2 years ago

combined patch for slider tooltips, compare checkbox, and loading status

comment:11 follow-up: @jrbeilke2 years ago

Ok, this new patch should take care of points 1 and 4.

I've modified the slider tooltips based on an upcoming jQuery UI 1.11 feature (3071) for slider labels. This allows the tooltip/label to follow the slider and should take care of the positioning issues.

That leaves point 2 for adamsilverstein to wrap up, and I've left out point 3 for now.

comment:12 @jrbeilke2 years ago

ahoereth on IRC reported an issue with the two revision columns being slightly different sizes and causing words to wrap differently (http://d.pr/i/1hdk).

This issue appears to be related to the percentage widths used for the revisions table, including the middle spacer column (48% / 4% / 48%), where the browser is rounding things differently. By setting the middle spacer column to width: auto we can maintain the width of the left and right column, and let the browser use the middle column to fill in the remaining space.

Last edited 2 years ago by jrbeilke (previous) (diff)

@jrbeilke2 years ago

updating my earlier patch to include a fix for the revision column widths bug

comment:13 in reply to: ↑ 11 @adamsilverstein2 years ago

Replying to jrbeilke:

Ok, this new patch should take care of points 1 and 4.

I've modified the slider tooltips based on an upcoming jQuery UI 1.11 feature (3071) for slider labels. This allows the tooltip/label to follow the slider and should take care of the positioning issues.

That leaves point 2 for adamsilverstein to wrap up, and I've left out point 3 for now.

thanks for the patch... wanted to clarify the tooltip issue:

i think we want to attack the tooltips to the actual tickmarks, sorry if my original comment was confusing. i thought the tooltips went with the handle and thats how i originally coded it, but it makes more sense if they are over the ticks, then you can hover over the ticks to see which revision they represent even before moving the handle there. do you want to take a shot at that?

comment:14 @SergeyBiryukov2 years ago

A note on formatting: per our CSS Coding Standards, each selector should be on its own line.

@jrbeilke2 years ago

split css selectors to separate lines per the coding standards

@ocean902 years ago

comment:15 follow-up: @ocean902 years ago

  • Keywords has-patch added; needs-patch needs-ui removed

23935.patch:

  • Continues 23935.tooltips-checkbox-status.3.patch
  • Moves color declarations to classic/fresh styles
  • Syncs classic with fresh stylesheet
  • Fixes a bug in jrbeilke's patch on tooltip resetting
  • Aligns the status message to the left
  • Restores the color shades for scope of changes
  • Styles diff header

comment:16 in reply to: ↑ 15 @adamsilverstein2 years ago

looks good :)

i am working on moving the tooltips to the ticks instead of the slider handles as suggested in several places; will post a patch back here.

Replying to ocean90:

23935.patch:

  • Continues 23935.tooltips-checkbox-status.3.patch
  • Moves color declarations to classic/fresh styles
  • Syncs classic with fresh stylesheet
  • Fixes a bug in jrbeilke's patch on tooltip resetting
  • Aligns the status message to the left
  • Restores the color shades for scope of changes
  • Styles diff header

comment:17 @ocean902 years ago

In 24119:

Revisions UI update:

  • Style status loading as a regular update message like on the other screens
  • Move compare two checkbox to diff header to avoid an overlap on long post titles
  • Restore color shades for the scope of changes visualisation
  • Show tooltips while moving the slider handle
  • Diff header styling
  • Move color declarations to classic/fresh stylesheets
  • Sync admin color stylesheets

props jrbeilke for initial patch. see #23935.

comment:18 @ocean902 years ago

  • Description modified (diff)
  • Keywords needs-patch added; has-patch removed

Okay, so let's use this ticket now for tooltip improvements.

comment:19 @adamsilverstein2 years ago

23935-improved-tick-marks.diff:

  • change language 'Calculating revision diffs' => 'Calculating revision comparisons'
  • move tooltips from slider handles to tick marks, hovering over the slider bar shows meta info from nearest tick mark
  • add click action - clicking in slider bar moves handle to that position; clicking to the left of the left handle in the two handle mode moves the left handle, clicking between the handles or to the right of the right handle moves the right handle (reversed in RTL mode)
  • re-work spacing technique for tick marks using borders instead of margins (required so hover action works), even borders left/right so handle centered on tick marks (previously handle was off on wider ticks)
  • js variable name cleanup (consolidate var statements)
  • css cleanup, remove tick mark margins, rework colors; add back scope-of-changes-none weight

comment:20 @adamsilverstein2 years ago

i've been feeling a bit like the guy who killed pluto since i removed the revisions easter egg someone worked so hard on. restore-eater-egg.diff restores the possibility of running that code, but won't be reachable accidentally (i'm guessing you can see how to activate it), a previous complaint. i've been hunting for an acceptable way to re-enable, this is the 1st that actually made sense; enjoy!

comment:21 @adamsilverstein2 years ago

23935-improved-tick-marks.3.diff​

  • reposition loading spinner, remove wording - spinner conveys enough meaning;
  • position absolute so display doesn't jump around when loading

@adamsilverstein2 years ago

add arrow below tooltips

comment:22 @adamsilverstein2 years ago

23935-improved-tick-marks.4.diff​
*adds arrow back for tooltips
*clean up alignment on tooltip text

http://f.cl.ly/items/2n0o2L1R212v2y093l2K/Screen%20Shot%202013-05-01%20at%201.54.53%20PM.jpg

comment:23 follow-up: @ocean902 years ago

@adamsilverstein: The range background color is missing, is this by design?

comment:24 in reply to: ↑ 23 @adamsilverstein2 years ago

Replying to ocean90:

@adamsilverstein: The range background color is missing, is this by design?

i think the range is now being hidden with the new border method for the ticks; wasn't intentional, but sort of prefer that way.

@adamsilverstein2 years ago

remove color for range

@adamsilverstein2 years ago

add color-classic.css

comment:25 follow-ups: @ocean902 years ago

23935-improved-tick-marks.7.diff:

  • Adds tooltip background color to classic style
  • Adds for the first and the last tick border-radius 3px since the slider has it too. Otherwise it will overlap the slider
  • I have increased the width of the ticks by 1px. scope-of-changes-none has 1px, scope-of-changes-vsmall has 2px and so on.

Issues:

  • Because we have now scope-of-changes-none with 1px which will be extended to 2px+ we should remove the style declarations for completed-false and completed-true since it's not visible anymore and the width increase seems good enough for a loading animation. But we can leave the classes.
  • In single mode the last tick gets always the .scope-of-changes-none class

comment:26 in reply to: ↑ 25 @adamsilverstein2 years ago

Replying to ocean90:

23935-improved-tick-marks.7.diff:

  • Adds tooltip background color to classic style
  • Adds for the first and the last tick border-radius 3px since the slider has it too. Otherwise it will overlap the slider

good catch

  • I have increased the width of the ticks by 1px. scope-of-changes-none has 1px, scope-of-changes-vsmall has 2px and so on.

great.

Issues:

  • Because we have now scope-of-changes-none with 1px which will be extended to 2px+ we should remove the style declarations for completed-false and completed-true since it's not visible anymore and the width increase seems good enough for a loading animation. But we can leave the classes.

the 'completed' class adds an animation to the tick mark while the underlying revision diff is loaded, don't think changing width really works as well may be enough though.

  • In single mode the last tick gets always the .scope-of-changes-none class

this is usually the 'current' revision, is the scope still 'none' even if there are several changes in the current version? if so this is a bug and i will dig deeper.

@adamsilverstein23 months ago

fixes last tick mark scope of changes not loaded correctly, fixes slider width calculation

comment:27 in reply to: ↑ 25 ; follow-up: @adamsilverstein23 months ago

Replying to ocean90:

23935-improved-tick-marks.7.diff:

  • Adds tooltip background color to classic style
  • Adds for the first and the last tick border-radius 3px since the slider has it too. Otherwise it will overlap the slider
  • I have increased the width of the ticks by 1px. scope-of-changes-none has 1px, scope-of-changes-vsmall has 2px and so on.

Issues:

  • Because we have now scope-of-changes-none with 1px which will be extended to 2px+ we should remove the style declarations for completed-false and completed-true since it's not visible anymore and the width increase seems good enough for a loading animation. But we can leave the classes.
  • In single mode the last tick gets always the .scope-of-changes-none class

23935-improved-tick-marks.8.diff:

  • fixes last tick mark scope of changes not loaded correctly
  • fixes slider width calculation by including left, right tick widths in calculation

http://f.cl.ly/items/0W0v2k2Q1r0w36132J12/Screen%20Shot%202013-05-03%20at%209.41.53%20AM.jpg

comment:28 in reply to: ↑ 27 ; follow-up: @ocean9023 months ago

Replying to adamsilverstein:

The calculation doesn't work pretty, see http://cl.ly/OjuG (in single mode too)

@adamsilverstein23 months ago

fix width calculation

comment:29 in reply to: ↑ 28 @adamsilverstein23 months ago

Replying to ocean90:

Replying to adamsilverstein:

The calculation doesn't work pretty, see http://cl.ly/OjuG (in single mode too)

think it was odd/2 calculation issue; changed from floor to ceil, please try 23935-improved-tick-marks.9.diff see if problem goes away

comment:30 @ocean9023 months ago

In 24174:

Revisions UI: Just another update - Now with improved tick marks and tooltips.

props adamsilverstein. see #23935.

comment:31 @adamsilverstein23 months ago

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

comment:32 @adamsilverstein23 months ago

  • Keywords needs-patch removed

comment:33 @ocean9023 months ago

  • Description modified (diff)

We decided to not bring back the easter egg (Huh, which easter egg…?).

Note: See TracTickets for help on using tickets.