WordPress.org

Make WordPress Core

Opened 3 years ago

Last modified 2 years ago

#33830 assigned defect (bug)

Revisions: Compare revisions has layout issues on iPhone 6+

Reported by: ryan Owned by: adamsilverstein
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 4.3
Component: Revisions Keywords: make-flow has-patch reporter-feedback
Focuses: ui Cc:

Description

From the editor, tap a revision in the Revisions meta box. The resulting compare revisions screen has some layout issues. Restore this Revision is obscured.

Attachments (7)

IMG_4715.PNG (167.6 KB) - added by ryan 3 years ago.
iPhone 6+
33830.patch (853 bytes) - added by PranaliPatel 3 years ago.
33830.2.patch (464 bytes) - added by karinedo 3 years ago.
33830.3.patch (529 bytes) - added by karinedo 3 years ago.
33830.4.patch (1.3 KB) - added by PranaliPatel 3 years ago.
This patch should fix spacing issue for mobile screen
33830.5.patch (1.3 KB) - added by adamsilverstein 2 years ago.
33830.6.patch (1.3 KB) - added by karinedo 2 years ago.

Download all attachments as: .zip

Change History (16)

@ryan
3 years ago

iPhone 6+

@PranaliPatel
3 years ago

#1 @PranaliPatel
3 years ago

  • Keywords has-patch added

#2 @adamsilverstein
3 years ago

Thanks for the bug report @ryan and patch @PranaliPatel!

I tested the patch with an emulator and this definitely fixes the button placement on mobile - much better! However I am seeing some side effects: in particular, the tooltips are now off. I haven't tested the slider on an actual mobile device to see how it works - do you even see the meta popups? @PranaliPatel can you test try to address that in a new patch?

Before patch:

http://cl.ly/image/1Z3d1i2V360I/Image_2015-10-03_at_5.25.47_PM.png_6221020_2015-10-03_17-28-37.jpg

After patch, note tooltips overlap slider: http://cl.ly/image/2t0y31020v14/Image_2015-10-03_at_5.26.41_PM.png_6941082_2015-10-03_17-29-39.jpg

@karinedo
3 years ago

#3 @karinedo
3 years ago

This patch should fix the restore button and tooltip positions on mobile

#4 @adamsilverstein
3 years ago

  • Keywords needs-patch added; has-patch removed
  • Owner set to adamsilverstein
  • Status changed from new to assigned

@karine: Thanks for the patch!

That fixes the placement of the button, yea!

Now I see another issue: The slider gets obscured by previous/next on small screen:

http://cl.ly/image/343K3F3B171q/Revisions__sadasd__WordPress_2015-10-06_16-47-32.jpg

I see this up to medium sized screen widths, it starts to look fine around iphone 6 landscape size (568w) in my brief testing.

The slider width is getting set in JS, and is clearly making some incorrect calculations. I can look into that, it should work fine at a smaller size.

Could we use CSS to change the next/back buttons content to icons, maybe arrows, below a certain breakpoint? Thinking these buttons could be even larger in other languages, so subbing them for icons would make sense for small screens.

@karinedo
3 years ago

#5 @karinedo
3 years ago

The slider width can be reduced for small screens, but when I tried keeping it between the buttons, it didn't look very good (unbalanced)... Patch puts the buttons above the slider on mobile

#6 @adamsilverstein
3 years ago

  • Keywords has-patch reporter-feedback added; needs-patch removed

Looks great @karinedo - thanks! Much, much better than before.

Can you add some more space above the slider? It looks unbalanced, seems to be more space below than above the slider. What do you think?

Here is a screenshot in chrome debugger on iphone 6 size screen:

http://cl.ly/image/3C1t1A2j0Y3I/Revisions__sadasd__WordPress_2015-10-06_22-27-09.jpg

@PranaliPatel
3 years ago

This patch should fix spacing issue for mobile screen

#7 @adamsilverstein
3 years ago

@PranaliPatel - Thanks for the patch!

I finally got to testing and the slider spacing looks better, however the tooltips didn't get the same padding and now float above the slider bar:

http://cl.ly/image/0R1A1C2J1l1d/Revisions__This_is_nice__WordPress_2015-10-26_11-25-50.jpg

#8 @adamsilverstein
2 years ago

33830.5.patch

  • Refresh against trunk

@karinedo
2 years ago

#9 @adamsilverstein
2 years ago

Thanks @karinedo! 33830.6.patch fixed the placement of the tooltip in my testing:

Browserstack/Iphone6s: http://cl.ly/123v3C2Y0K1b/Dashboard_2016-04-27_13-43-39.jpg

Note: See TracTickets for help on using tickets.