#31195 closed feature request (fixed)
Add a user-friendly way to preview site responsiveness in the Customizer
Reported by: | celloexpressions | Owned by: | westonruter |
---|---|---|---|
Milestone: | 4.5 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Customize | Keywords: | has-screenshots has-patch commit |
Focuses: | ui, accessibility, administration | Cc: |
Description
Now that nearly every theme/site has a responsive design, the lack of a user-oriented UI for previewing a site on different device sizes is a common complaint. Yes, it's as simple as resizing the browser window, but many user's don't think of that.
I've prepared a conceptual patch that replaces the Customizer's "collapse" link with a series of buttons that change the size of the preview window to emulate different device sizes. This would ideally integrate with #29949, providing a new full-screen/collapsed mode that's a bit more user-friendly, and could potentially offer the ability to have a "sticky" full-screen mode, where the Customizer controls pop out over the preview window instead of shrinking it.
Many of these ideas have been explored in the WordPress.com skinning of the Customizer, largely unsuccessfully (depending on who you ask). But I think a carefully considered approach here could be a big benefit for core.
Attachments (29)
Change History (134)
#1
follow-ups:
↓ 2
↓ 5
@
10 years ago
Many of these ideas have been explored in the WordPress.com skinning of the Customizer, largely unsuccessfully (depending on who you ask). But I think a carefully considered approach here could be a big benefit for core.
I'd argue that responsiveness is one of the things that .com gets right in many ways. I have to say I really like your initial approach much better, though.
(Adding WordPress.com responsive demo for future reference.)
#2
in reply to:
↑ 1
@
10 years ago
Replying to philiparthurmoore:
I'd argue that responsiveness is one of the things that .com gets right in many ways. I have to say I really like your initial approach much better, though.
Sorry, I should have worded that better. I think my general complaint is the overall reskinning and especially the pop-over controls behavior (essentially a sticky full-width preview mode), but I definitely like the idea of the responsive toggles, hence this ticket :). The fact that pretty much none of .com's customizations have even been proposed for core after nearly three years indicates that there are major issues with the overall approach there, I think, not to say that the core version doesn't have issues.
This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.
10 years ago
#4
@
10 years ago
@celloexpressions How do you feel about putting this into a plugin so some of us can use it against users and gauge reactions?
#5
in reply to:
↑ 1
@
10 years ago
Replying to philiparthurmoore:
Many of these ideas have been explored in the WordPress.com skinning of the Customizer, largely unsuccessfully (depending on who you ask). But I think a carefully considered approach here could be a big benefit for core.
I'd argue that responsiveness is one of the things that .com gets right in many ways. I have to say I really like your initial approach much better, though.
(Adding WordPress.com responsive demo for future reference.)
Note that the WordPress.com Customizer skin has been reverted, except for the Device Preview plugin, which I agree is a nice addition.
BTW, in lieu of this WordPress.com Customizer Device Preview plugin being available in VIP Quickstart, I've put together a temporary standalone plugin that makes it available on any WordPress install: https://github.com/xwp/wp-customizer-device-preview
As an aside, we've got a VIP client for whom we're going to be extending the Device Preview plugin to change the current previewUrl
when the selected device changes so that the selected device can be passed as a query var for the URL loaded in the Customizer preview, allowing us to properly preview an adaptive (non-responsive) theme, i.e. when jetpack_is_mobile()
.
Any device preview functionality added to the Customizer should allow a theme to indicate whether it is completely responsive, or if it has adaptive elements that necessitate a URL change when toggling between desktop/mobile.
#6
@
10 years ago
- Milestone changed from Awaiting Review to Future Release
How about working on this as a feature for 4.3?
#7
follow-up:
↓ 8
@
9 years ago
Attached a quick iteration (i2) on @celloexpressions' concept above:
- Icons now at pixel-perfect size.
- The collapse icon is now kept in the same position, so you can quickly switch between collapsed and non-collapsed mode.
- Used tones of grey to adapt better to the color scheme and highlight the active option.
- Used a black background instead of the frame for the responsive previews.
I hope this helps. :)
#8
in reply to:
↑ 7
@
9 years ago
Replying to folletto:
- The collapse icon is now kept in the same position, so you can quickly switch between collapsed and non-collapsed mode.
The collapse/expand icons are a bit too close to a "full screen" metaphor, which isn't what they do. They're really "Show Customizer" and "Hide Customizer" icons. Is there a better icon we can use?
#9
follow-up:
↓ 10
@
9 years ago
The collapse/expand icons are a bit too close to a "full screen" metaphor, which isn't what they do. They're really "Show Customizer" and "Hide Customizer" icons. Is there a better icon we can use?
I agree. Not sure of a better choice. We can make new ones, or use a left/right arrow, which however I feel might be less clear.
#10
in reply to:
↑ 9
@
9 years ago
Replying to folletto:
I agree. Not sure of a better choice. We can make new ones, or use a left/right arrow, which however I feel might be less clear.
Left/right arrow might work in combination with a "sidebar"-like icon? We should get the dashicons team to explore some ideas. :)
#11
@
9 years ago
@folletto's design is now implemented in the Customizer UI Experiments plugin for ease of testing and iterations. If anyone wants commit access there, let me know and I'll add you.
We need to figure out what to do in terms of the colors of the buttons, as we can't go too light on the disabled state while meeting accessibility/color contrast guidelines. For now I kept the collapse button as a separate arrow, since that was just redone in core to be more accessible. @designsimply has mentioned in the past that the "collapsed" text tends to do very well in user testing, although I would like to simplify to just an icon if we can find the right icon. The design of that could use tweaking to better match the device-preview buttons. I'm also not sure about the background color - it's a bit strong even at the darkest WordPress gray, and clashes badly with dark themes. We won't be able to make it look good with every theme here, so we need to decide whether it makes more sense to go dark, light, or in the middle depending on how we want to contextualize the preview. I'm currently leaning toward trying the admin bar background (the tinted version of #222
).
#12
@
9 years ago
While I entirely agree that text would make it super clear, I also think that this specific feature isn't critical, so even if it's slightly more obscure (being just an icon) it's ok. :)
I agree we need a proper icon tho. :)
I did some super quick sketching, reusing the same shape icons we already use elsewhere. Four ideas:
The color isn't solvable in that regard. To be visible everywhere, requires a color fill on one edge of the brightness spectrum and a border on the other edge. Which leads to a series of usual solutions, but can't really be worked around. The idea as the above is to have a design that looks detached enough on same-color (i.e. adding the shadow to the white-on-white) and is nice even if contrasting on inverted-color (i.e. white-on-black fill).
#13
@
9 years ago
Wee need design and accessibility review on the plugin version. If it's okay, I can make a merge patch for 4.4. I think we should split changes to the collapse button to a separate ticket since the current deisgn doesn't change that component.
#15
@
9 years ago
Wee need design and accessibility review on the plugin version.
I did a test run. My feedback would be:
- Remove
border-radius
andbox-shadow
- Either user the WP Admin color scheme accent color (active button color) on the active one -or- make the grey of the inactive icons lighter.
This because the round circle looks a bit off (not used anywhere else), clashes with the top border, and adds visual noise. Using just color in that way I think it's also doable in terms of accessibility because it's something discoverable. If however we want a shape marker also, let's use a highlight line, like this (still accent color, 3px):
---
While I just did UX / design review, not browser testing, I however noticed that the collapse/responsive bar disappears on certain screen sizes... wider/taller than I would have expected make it disappear. Not a big issue, but 1024x800 (a small desktop screen, but still desktop) disappears. If we can update this, we should make the responsive buttons disappear at decreasing width (i.e. don't show tablet size if it's smaller than a tablet).
In terms of vizrec I defer to Ryan's post above. :)
Ok splitting the collapse icon in a separate ticket. :)
#16
@
9 years ago
@aferica - would you agree that @folletto's proposed changes still provide adequate distinction for accessibility purposes, assuming the focus style matches the active style? I'm hesitant to make the inactive icons lighter, as they'd then no longer meet color contrast guidelines.
#17
@
9 years ago
- Focuses accessibility added
@celloexpressions color alone cannot be the only indicator, I'd recommend a shape marker whether it's an outline at the bottom or a circular thing. Both the shape and the inactive icons should have a sufficient color contrast.
This because the round circle looks a bit off (not used anywhere else),
For the focus part this is probably going to change :) see #33808. Not used anywhere as "active" indicator though.
#19
@
9 years ago
I would like to add my 2 cents to this discussion. @celloexpressions said that previewing and customizing could be as simple as resizing the browser window, but I think some use cases are forgotten here.
First, @westonruter mentioned adaptive websites that needs a full page refresh to load different markup for small screens. But I would like to go a little bit further. As a theme author, I want to let user decide the layout of their site in the customizer, and this layout can be different for small and wide screens. So changing the size of the preview window could also toggle some controls that are specific to the screen size.
Let me take an example : in Bootstrap 3 grid system, you can define columns using classes like col-md-4
(that will make a 4/12 width column on medium size display) and col-xs-6
(that will make a 6/12 width column on very small size display). So I would like a slider to define the number of columns on medium size display, and another slider to define columns on a very small size display. Adaptive website would have the same needs : some controls in the customizer could be only for the mobile version of the site.
Second : how will you determine the sizes breakpoints ? Will you use iPhone / iPad / iMac typical display sizes ? But we now have plenty of different display sizes. So that could be nice to let theme author define the size breakpoints themselves (there are the one that know which size needs previewing). I would even add a new requirement : why not enable people that have medium size displays to preview their site on a wide display ? That's possible if you extend the size of the iframe and use scrollbars.
#20
@
9 years ago
For the record, both Firefox and Chrome propose a user interface to test the responsiveness of a website. Even if those tools are targeted to a tech-savvy audience, we could get some inspiration from it.
Both tools have a drop-down menu to let user choose a predefined size (Firefox just give sizes, whereas Chromium propose device names). There is also the possibility to manually enter a specific size.
This type of user interface would allow theme and plugin authors to add custom sizes easily. That would also remove a misunderstanding : the only thing that is previewed here is the screen size. The website can behave differently on a real mobile device (ex: videos and audios, form controls, ...)
#21
@
9 years ago
I think that there should be default breakpoints defined for Desktop, Tablet, and Mobile. The configuration for each breakpoint could include the label, icon(?), width/height, and whether entering the breakpoint requires a refresh (i.e. it is adaptive not responsive):
array( array( 'id' => 'desktop', 'label' => __( 'Desktop' ), 'width' => 1366, 'height' => 768, 'refresh' => false, /* responsive */ ), array( 'id' => 'tablet', 'label' => __( 'Tablet' ), 'width' => 768, 'height' => 1024, 'orientation' => 'landscape', 'refresh' => false, /* responsive */ ), array( 'id' => 'mobile', 'label' => __( 'Mobile' ), 'width' => 360, 'height' => 600, 'orientation' => 'portrait', 'refresh' => true, /* adaptive */ ) )
(Something we haven't talked about yet is previewing different device orientations, e.g. landscape instead of portrait.)
The theme can then filter this list of defaults based on which breakpoints it supports.
I should also note that customizer transactions (#30937) will make it possible to pop the preview frame out into a separate window, and make it easier to view the pending setting changes using those responsive previews, or even on other devices entirely.
#22
@
9 years ago
Thanks @westonruter for your answer. Your solution would make it possible to add different parameters for portrait / landscase view without the orientation
parameter :
array( array( 'id' => 'mobile-portrait', 'label' => __( 'Mobile - portrait' ), 'width' => 360, 'height' => 600, ), array( 'id' => 'mobile-landscape', 'label' => __( 'Mobile - landscape' ), 'width' => 600, 'height' => 360, ) )
And what is your opinion about having customizer controls that hide or show depending of the preview size (see my first point above) ?
#23
@
9 years ago
Please note that:
- The feature should be simple. Portrait-Landscape usually makes the theme fall into the intermediate buckets (i.e. a landscape tablet is basically a desktop, a landscape phone is basically a portrait tablet). This feature is meant to be a quick preview, not a technical tool.
- On desktop, where this feature shows, you are going to have a resizable window, so if you really need to, you can just resize the window. Or use the Inspector as shown above. In other words: this feature is meant to be quick, if you want precision, you already know and have better tools to do that, and we shouldn't replicate these tools (I know, already discussed above, but I'll still point it out ;) ).
- I disagree with "let theme author define the size breakpoints " exactly because "we now have plenty of different display sizes". The theme adapts to screens, not the other way around. Thus the responsive UI is something that exists independently from the themes, and should be set at proper sizes determined by industry averages. Themes should not have any bearing on the responsive UI. This will have also the ripple effect of helping more theme authors to see the industry averages, exactly because we provide a good way to preview them.
That said:
why not enable people that have medium size displays to preview their site on a wide display ? That's possible if you extend the size of the iframe and use scrollbars.
I agree. This is definitely something I'd go for, even if likely in a separate issue and discussion, because I'd still start with this UI and then work to achieve that.
The logic behind that would be to have the controls always visible (so, no hiding) but then it detects the screen size and defaults to that, and the other options then will grow/shrink accordingly.
So changing the size of the preview window could also toggle some controls that are specific to the screen size.
This could be very useful, and should be a separate ticket imho to discuss it. ;)
#24
follow-up:
↓ 25
@
9 years ago
I agree with @folletto - at least for a first pass we should strive for simplicity. Core sets the breakpoints, there are only a couple of options, etc.
If the screen is larger than mobile (I believe google/andriod considers 600px wide to be the standard phone/tablet distinction point) but the preview less than, say, 1024px, I think it would make sense to introduce scrollbars in the preview so that a larger preview can be seen.
In terms of allowing sites to force a refresh if needed or have different sets of controls, perhaps sending an event to the preview and the controls when the size button is selected is enough. Then, themes and plugins can listen for that event and react accordingly depending on their specific needs.
#25
in reply to:
↑ 24
@
9 years ago
Thanks everybody for your answers.
Replying to celloexpressions:
perhaps sending an event to the preview and the controls when the size button is selected is enough.
Yes, that would be great ! Sending an event with the selected size as a parameter would enable theme and plugin authors to hide or show controls when needed.
Replying to folletto:
why not enable people that have medium size displays to preview their site on a wide display ? That's possible if you extend the size of the iframe and use scrollbars.
I agree. This is definitely something I'd go for, even if likely in a separate issue and discussion, because I'd still start with this UI and then work to achieve that.
I have created a new ticket for that : #34051.
#27
@
9 years ago
The scope of #34051 could be covered here in the context of devices where you're narrower than a reasonable minimum laptop size (although this would probably need to be default behavior and could thus introduce complexity). However, I'm not sure that forcing horizontal scrollbars on the frontend by default is the best idea - that could seem equally broken to triggering tablet mode in certain situations depending on the theme. It may be better to split that off as separate.
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
9 years ago
#30
@
9 years ago
@folletto Huge +1 to allowing desktop views on medium or even small screens via giant scrollable frame. This will be super important for those creating or more likely modifying sites entirely on smaller devices. I've been noodling on good ways to do this. Getting a good view of everything at once is still important so maybe a scaled down version might be something to consider. That said, for this particular initial implementation, it should be kept as simple as possible. Everything I've seen above looks pretty nice so far.
#31
@
9 years ago
Not related to design choices: had a quick look at how this was implemented on wp.com and from a semantic/accessibility point of view I'd recommend to use elements that natively support keyboard interaction (e.g. buttons) instead of <span>
s :(
#32
follow-up:
↓ 33
@
9 years ago
- Keywords needs-patch added; has-patch removed
The version I've been working on (in the Customizer UI Experiments feature-plugin) uses buttons, as well as screen reader text for the icons. Should we also use a11y.speak() here?
I've just refreshed the plugin to use clearer focus styles within the circular box-shadow language. Can anyone with opinions here test that out and offer suggestions as to whether we should stick with that direction or try the underlines as suggested by @folletto above? I think the circles work nicely and are a good option in terms of accessibility.
@westonruter and I also discussed how best to allow themes to hook into preview changes, his suggestions on slack can be implemented once we finalize the UI and move into patches (https://wordpress.slack.com/archives/core-customize/p1449868495000210).
In terms of doing a larger scrollable preview on smaller devices, it's definitely worth exploring, but I'm not sure whether it makes more sense to land a basic version first and then explore that as an extension, or make it part of the initial proposal. Anyone have specific thoughts on how we might define how that works - maybe defaulting to "tablet" mode if the device is within certain range limits and then forcing it larger when the desktop icon is clicked?
If we're ready to move forward, I can convert the plugin to a patch (device-preview component only). Related, I can add anyone who wants to play with the code to that plugin's committer list. Also, I saw some mockups from @melchoyce that moved these around; I'm thinking that the bottom of the controls is the best spot for now and they should be easy to move if other design changes facilitate that later on.
#33
in reply to:
↑ 32
@
9 years ago
Replying to celloexpressions:
Should we also use a11y.speak() here?
@celloexpressions thanks :) played a bit with the plugin, and I'd suggest to consider some improvements.
1 - Ideally, in the markup the buttons should be before the "Collapse Sidebar" button simply because when navigating through elements using the keyboard and a screen reader I would probably assume the "Collapse Sidebar" being the last control in the UI.
2 - Consider to add a hidden heading before the buttons to identify this section
3 - Probably I wouldn't use a11y.speak() here. There is the need to give feedback about which of the buttons is the currently active one and feedback after a button is pressed. I'd probably use aria-pressed. Here's how the buttons are announced now using Firefox+NVDA:
Enter desktop preview mode button Enter tablet preview mode button Enter mobile preview mode button
When activating one of the buttons, there's just silence. Using an aria-pressed="true/false"
attribute to be dynamically updated things get better:
Enter desktop preview mode toggle button pressed Enter tablet preview mode toggle button Enter mobile preview mode toggle button
The buttons are now announced as toggle buttons and the active one as "pressed". When activating one of the buttons, NVDA will announce the state change saying just "pressed". I'd also like to hear the accessibility team opinion and will ask the team to have a look at the plugin.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
9 years ago
#35
@
9 years ago
Also, I saw some mockups from @melchoyce that moved these around; I'm thinking that the bottom of the controls is the best spot for now and they should be easy to move if other design changes facilitate that later on.
Agreed. The referenced mockups are an exploration I'd consider separate from the initial goal of getting device previewing into the Customizer.
#36
@
9 years ago
Here's a plugin based on the same idea by @webnist .
https://wordpress.org/plugins/customizer-theme-resizer/
(1 minute demo video: https://www.youtube.com/watch?v=_DFdXqbeRPQ )
- size choice by phone names.
- able to change width height manually.
- refresh button
- vertical/horizontal switch button
#37
@
9 years ago
That's far, far too complicated for a quick preview tool and reduces preview space too. Plus, phone names are meaningless to normal people unless your phone is in the list.
That's plugin realm in my view.
That said, for this particular initial implementation, it should be kept as simple as possible. Everything I've seen above looks pretty nice so far.
+1.
This ticket was mentioned in Slack in #accessibility by afercia. View the logs.
9 years ago
#39
@
9 years ago
- Owner set to celloexpressions
- Status changed from new to assigned
I'm going to convert the plugin to a patch and incorporate the latest feedback regarding accessibility and providing hooks for themes to respond to preview size changes with JS. Then we can do any user testing and adjustments that we want to additionally consider before getting a first pass in.
I reopened #34051 to track the idea of forcing a larger size than available, so that we can treat that as a separate enhancement to make after the initial ability to preview smaller sizes from larger devices is introduced. Discussion on that approach can start there now.
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
@
9 years ago
Mobile preview with 31195.2.diff. The circle shadow goes blue on hover/focus regardless of the active status, the active item remains gray.
#41
@
9 years ago
- Keywords has-patch has-screenshots added; needs-patch removed
31195.2.diff brings this back into patch form with the accessibility improvements suggested above and the finalized design. I started adding the way for developers to hook into changes but it's not quite working yet. In terms of UI and accessibility though, it's pretty polished.
One issue I'll note is that when navigating to different pages with the mobile preview, the reloaded/new page briefly shows up under the active preview when it loads. Transactions will be changing the behavior there anyway, so I think it's fine to leave for now.
@afercia I tried adding <h3 class="screen-reader-text"><?php _e( 'Device Previews' ); ?></h3>
right inside the devices div, but it caused the expand/collapse animation to break and I'm also not sure whether it's better or not. The rest of the general customizer buttons (help, save, close, collapse, etc.) don't have headings, so it seems like it may be confusing to only have them for this one section (although these three buttons are more directly related than any of the others).
#42
follow-up:
↓ 43
@
9 years ago
- Keywords needs-patch added; has-patch removed
@celloexpressions I've provided inline feedback here: https://github.com/xwp/wordpress-develop/pull/138/files
One issue I'll note is that when navigating to different pages with the mobile preview, the reloaded/new page briefly shows up under the active preview when it loads. Transactions will be changing the behavior there anyway, so I think it's fine to leave for now.
Can you screencast this? I'm not seeing it.
#43
in reply to:
↑ 42
@
9 years ago
Replying to westonruter:
@celloexpressions I've provided inline feedback here: https://github.com/xwp/wordpress-develop/pull/138/files
One issue I'll note is that when navigating to different pages with the mobile preview, the reloaded/new page briefly shows up under the active preview when it loads. Transactions will be changing the behavior there anyway, so I think it's fine to leave for now.
Can you screencast this? I'm not seeing it.
Thanks! I've added a screencast showing the preview issue.
#44
follow-up:
↓ 45
@
9 years ago
From the screenshots, looks good. Nice work. :)
I'd just suggest to not use the circle because clashes with the borders all around it. We can instead use the accent color "indent" for the selected item:
This allows both the shape marker useful to make it accessible, repeats a pattern we started using, and the user color (in the screenshot it's orange assuming the user has that color scheme, would be blue by default).
#45
in reply to:
↑ 44
@
9 years ago
Replying to folletto:
We can instead use the accent color "indent" for the selected item
Please consider color contrast should be at least 4.5:1 :)
#46
@
9 years ago
Please consider color contrast should be at least 4.5:1 :)
I assumed the accent colors from the color schemes to be already compliant. The idea of an accent is that it's visible. :D
Aren't they then? Not any of the colors?
#47
@
9 years ago
I ran a quick check.
0073aa
(default blue highlight) oneeeeee
(customizer background): 4.49 : 1dd823b
(sunrise color scheme) oneeeeee
(customizer background): 2.47 : 1656a65
(ring gray) oneeeeee
(customizer background): 4.71 : 1
So ok, looks to me not all the accent colors were adjusted for contrast. Note also that the ring gray, since it's a 1px round, in practice renders at a lighter gray due to anti aliasing (b4b4b4
) with a contrast of 1.79 : 1.
Dunno. 4.49 in the default color scheme looks close enough to 4.50 to me to be used, and seems definitely better than the anti-aliased ring even in this context. If that doesn't work, we can just revert for the highlight gray, still with the rectangular tab.
This ticket was mentioned in Slack in #core by westonruter. View the logs.
9 years ago
#49
@
9 years ago
Should the hover/focus style match the active style, or do we want to differentiate those? The current patch uses the standard focus highlight color on hover/focus (although there is no shape change so this isn't great for accessibility).
#50
@
9 years ago
For reference, I attached a screenshot of Press This UI, which is another UI that uses the same language suggested above. The full accent is used for keyboard navigation (focus) and the light one is for selected states. It doesn't seem to have hover states, so we might just use the rectangle also to indicate a hover.
#51
@
9 years ago
I added two mockups of how we might apply the press this border/background color for hover style to these buttons (I think the border should be one pixel narrower, at 4px). For the Customizer in core, the blue seems a bit too distracting (since we try to avoid introducing color here, with the save & publish button being the only colored element). I think the gray version could work well here though. Anyone else have thoughts on this?
#52
follow-up:
↓ 53
@
9 years ago
For Hover/Active we should use the same we're using for the x
at the top, since the bottom bar is structured in the same way, including the background gray.
Ok for the gray.
I've attached a second iteration (i2) that shows how it would work for Active / Hover.
Aside: from the screenshot, the icons seem a little too high (vertical alignment).
#53
in reply to:
↑ 52
;
follow-up:
↓ 54
@
9 years ago
I don't think the light gray background on hover is enough for hover/focus for accessibility (the Customizer back and close buttons are an issue that continues to be stalled in #29158). @afercia please correct me if I'm wrong - but we need to do more than that, right?
Replying to folletto:
Aside: from the screenshot, the icons seem a little too high (vertical alignment).
I think they are, I'll fix that.
#54
in reply to:
↑ 53
@
9 years ago
Replying to celloexpressions:
I don't think the light gray background on hover is enough for hover/focus
Yep, focus should not rely on color alone, needs a shape too.
#55
@
9 years ago
Yes, of course you're right.
So. The point is that the behaviour should be the same for the x
and the responsiveness toggles. I personally think we shouldn't make it different. Until it gets solved for both, I'd complete this ticket keeping that aligned, and then find a solution that stacks well for both the instances (x
and preview on top bar, collapse and toggles on bottom bar).
#56
@
9 years ago
Let's not introduce a new feature with an inaccessible design. There are many issues tying up a more accessible solution for the close button (in particular, the auto-focusing behavior that draws excessive attention to it), not to mention lack of movement on #29158, so tying this design to that would likely ensure that it remains inaccessible for several releases.
Instead, if my color-inversion gray proposal is too much for the device preview buttons, does anyone have other solutions for the hover/focus style there? We could use the active marker, but then there wouldn't be anything other than color for the active item being focused. That may be a good compromise, though.
#57
@
9 years ago
Let's not introduce a new feature with an inaccessible design.
I'm fine with that. I'm just saying that it should be the same solution, otherwise it starts getting way to many different styles all over the place, which is not ideal for a whole set of other reasons (and for everyone, using accessibility or not).
if my color-inversion gray proposal is too much for the device preview buttons, does anyone have other solutions for the hover/focus style there?
Note that it's not too much for keyboard active, it's too much for hover. ;)
Different solutions could be:
- The active is full dark, the hover is shaded (this assumes that hover doesn't have to be accessible since the mouse is over it, and we have to make sure that it never gets full dark in mouse interactions)
- The active/hover one is shaded and the icon gets bigger.
- The active/hover one is shaded and gets the same bottom rectangle as the selected one.
- The active/hover one is shaded and gets a half height (2px) bottom bar
- The active/hover one is shaded and the icon animates up (transition/jump) a few pixels.
I can make a mock of all these options (and other) later. ;)
#58
@
9 years ago
I like the idea of playing with the size of the bottom border since it's there anyway. Either make it 4px for focus and 2 for active or the other way around (thinking that it may be weird for the active tiem to get shorter when it's hovered/focused). We can animate it to add emphasis and potentially even use the blue for focus as a secondary differentiation, in addition to the light gray background.
#59
@
9 years ago
Ok. I tried the various options you mentioned and while playing with the rectangle height doesn't seem to work well, I feel that combining the various effects could work:
- Hover / Active have enough contrast thanks to the icon.
- The shape is there, even if of a different color.
- The selected one uses gray.
I attached i3 with that concept.
#60
@
9 years ago
While we're probably technically not quite there for distinguishing active vs. active and focused, I think this is good enough for now since focus and active styles are both independently clear relative to the normal display. I'll incorporate i3 into the next patch.
#61
follow-up:
↓ 63
@
9 years ago
Can we spin this into a feature plugin? The trac thread is getting long enough that parsing the state of the effort and what's left is non-trivial.
#62
@
9 years ago
While we're probably technically not quite there for distinguishing active vs. active and focused, I think this is good enough for now since focus and active styles are both independently clear relative to the normal display. I'll incorporate i3 into the next patch.
Ok, let's merge a couple of ideas. I've attached i4 that uses two different styles for hover and active. That should make keyboard navigation, which I feel it's more important since lacks cursor, clearly different.
Same note as before: it's important that the active state doesn't "bleed" into the hover one. :)
#63
in reply to:
↑ 61
;
follow-up:
↓ 67
@
9 years ago
There isn't a clean way to differentiate between hover and focus styles in CSS without making the focused element hidden from mouse interactions as far as I know. Ie, whenever one of these buttons is clicked, it will receive focus which would give it the blue background, and that would stick until the user clicks somewhere else. It's probably better to scale back the focus styling to match the hover styling rather than hacking in a way to prevent the buttons from receiving focus when clicked.
Replying to ericlewis:
Can we spin this into a feature plugin? The trac thread is getting long enough that parsing the state of the effort and what's left is non-trivial.
It was in a plugin, but the UI is ending up with more changes after we moved back to patch form. Other than this UI issue, I need to implement some technical feedback from westonruter to facilitate developers hooking into preview size change actions, and then I'll post a revised patch.
#64
@
9 years ago
It's probably better to scale back the focus styling to match the hover styling rather than hacking in a way to prevent the buttons from receiving focus when clicked.
Ok, makes sense. :)
This ticket was mentioned in Slack in #core by mike. View the logs.
9 years ago
#67
in reply to:
↑ 63
;
follow-up:
↓ 68
@
9 years ago
Replying to celloexpressions:
It was in a plugin, but the UI is ending up with more changes after we moved back to patch form. Other than this UI issue, I need to implement some technical feedback from westonruter to facilitate developers hooking into preview size change actions, and then I'll post a revised patch.
How is the revised patch going?
#68
in reply to:
↑ 67
@
9 years ago
Replying to westonruter:
How is the revised patch going?
I'm hoping to have it up by tomorrow evening.
#69
@
9 years ago
I was checking #29158 today and I noticed that we might want to make the icons width larger to have them squares instead of narrow rectangle. If possible feels would add a bit more balance to it. :)
#70
follow-up:
↓ 71
@
9 years ago
- Keywords has-patch added; needs-patch removed
31195.3.diff contains the revised design as well as previous adjustments from westonruter (updated screencast attached). I think we're looking pretty good here; I'm going to try to get a make/core post out about this tomorrow for broader feedback barring any major issues with this patch.
#71
in reply to:
↑ 70
@
9 years ago
Replying to celloexpressions:
31195.3.diff contains the revised design as well as previous adjustments from westonruter (updated screencast attached). I think we're looking pretty good here; I'm going to try to get a make/core post out about this tomorrow for broader feedback barring any major issues with this patch.
Looks good to me.
This ticket was mentioned in Slack in #core by mike. View the logs.
9 years ago
#73
follow-up:
↓ 74
@
9 years ago
On hover/focus the colors are:
background-color: #ddd; border-bottom-color: #0073aa;
resulting in a contrast ratio of 3.8:1 :(
#74
in reply to:
↑ 73
@
9 years ago
Replying to afercia:
On hover/focus the colors are:
background-color: #ddd; border-bottom-color: #0073aa;resulting in a contrast ratio of 3.8:1 :(
I would be more worried about the ratio of text to background (#191e23
to #ddd
, 12.36:1) and border to page background (#0073aa
to #eee
, 4.49:1) than border to focus background. Ultimately, users should be able to see the icon even when hovered/focused, and on focus at minimum the presence of a marker where there was previously background should be discernible even if it can't be distinguished from the adjacent background color change. Per the previous discussions, there are a few compromises here but I think there's a pretty good balance of considerations here now.
#75
@
9 years ago
We could then go gray if the starting point is white, and white if the starting point is gray?
I attached i5 to show the white highlight. :)
#77
@
9 years ago
- Keywords needs-docs added
PHP docs feedback on 31195.3.diff:
get_previewable_devices()
method:
- The DocBlock summary should use a third-person singular verb. A trick for getting this right is imagining how the information would be read in the context of the developer reference. Instead of "Get previewable devices.", should be something like "Retrieves a list of preview-able devices."
- The declaration should have a
public
access modifier, and the DocBlock should contain an@access public
tag - Needs an
@return
tag, type, and description ending in a period - The
customize_previewable_devices
filter needs an accompanying hook doc
Separately, we should probably escape $device
in the data-device
data attribute in wp-admin/customize.php
#78
@
9 years ago
- Keywords needs-docs removed
31195.4.diff contains the docs fixes, escaping on the attribute, and the white hover background color.
#79
@
9 years ago
In 31195.5.diff (diff):
- Refresh patch against
trunk
- Call
WP_Customize_Manager::get_previewable_devices()
only once instead of twice (reduces filter calls) - Simplify PHP template logic for preview buttons; use
esc_attr()
for escaping (instead of translation functionesc_attr_e()
) - Remove unused variable; de-duplicate jQuery selector queries; rework indentation for chained jQuery
There are two remaining issues I see before this is ready for committing:
- If I filter
customize_previewable_devices
to maketablet
ormobile
have thedefault
flag, then the iframe is not initially sized accordingly. - When previewing tablet or mobile and the preview iframe is not 100% height, there is a momentary doubling of the iframe when navigating around the site. This is the same issue discussed above in comment 43.
In regards to the doubling issue during navigating, the culprit can be found in this line of customize-controls.js
:
self.iframe = $( '<iframe />', { 'title': api.l10n.previewIframeTitle } ).appendTo( self.container );
This is called when the Ajax request to fetch the preview's contents finishes, but before the refreshed iframe's load
event is triggered, which is when the old iframe is removed. Notice how the iframe is appended to #customize-preview
as opposed to replacing the DOM element of the existing preview. I believe this is intentional, as if the new iframe fails to load, then the old one can remain in place. Ultimate the problem is that there is momentarily 2 iframes in the document, during the time between the Ajax request returning and the new iframe window triggering load
. The quickest fix I see is to just make sure the iframe
is absolutely-positioned so that the two iframes can momentarily overlap each other instead of momentarily stacking on top of each other:
-
src/wp-admin/css/customize-controls.css
p.customize-section-description { 610 610 #customize-preview iframe { 611 611 width: 100%; 612 612 height: 100%; 613 position: absolute; 613 614 } 614 615 615 616 .wp-full-overlay-sidebar {
Note that the WordPress.com plugin (customizer-device-preview.zip) doesn't seem to have this issue, so perhaps its CSS could be more closely examined to see how it handles that issue.
Aside: With transactions (#30937), the iframe won't get torn down and re-built with each request, so this issue won't be relevant when that lands. I'm not confident that it will land in 4.5, however.
#80
@
9 years ago
I believe .com has the preview full-height. Setting absolute positioning should work.
I suppose we can just run the js on init, or set the device class on the preview iframe in PHP.
#81
@
9 years ago
- Keywords needs-unit-tests added
Needs PHPUnit and QUnit tests as well. This should be straightforward since there isn't a whole lot of new code.
#82
@
9 years ago
In case anyone hasn't seen it, @celloexpressions wrote a post on Make Core for proposing this for merging: https://make.wordpress.org/core/2016/01/28/previewing-site-responsiveness-in-the-customizer/
This ticket was mentioned in Slack in #core by mike. View the logs.
9 years ago
#84
@
9 years ago
With apologies for the drive-by comment: is using in
as the measurement unit for the viewports deliberate? I imagine it is and understand the semantics - just if we're going to keep it that way, I'd love to see inline comments about it so we don't look at it later and wonder why it's that way. :)
#85
@
9 years ago
- Owner changed from celloexpressions to westonruter
I went with in
for two reasons - most device screen sizes are measured in (diagonal) inches in the US, so these sizes were roughly derived from a hypothetical device, and because that ends up being a pretty large unit that keeps us away from the precision of pixels. It's intentionally a fairly ambiguous size that isn't tied to pixels partially to minimize code-based reliance on specific pixel breakpoints when designing responsively and using the Customizer preview (I think I used pixels for mobile because 320px is generally a fairly standard lowest width, but that could also be changed). Of course, it could be changed very easily, or a code comment could be added.
The position: absolute
and default class on the preview should be pretty quick fixes, and I don't do unit tests, so I'm passing this off to finish up and commit.
#86
@
9 years ago
Tested this and it works really well! I like the simple extendable approach, nice work. Lets get some unit tests and I'm all for merging; seems quite useful!
#87
@
9 years ago
31195.6.diff: Add some escaping for (filterable) $settings['label']
. Diff - https://cloudup.com/cXn3l6azAa2
#88
follow-up:
↓ 89
@
9 years ago
Running the full suite of unit tests has a failure on Tests_WP_Customize_Manager::test_customize_pane_settings
. I have confirmed that it's not due to the new tests in my patch. I am looking into it further.
Full error:
1) Tests_WP_Customize_Manager::test_customize_pane_settings Failed asserting that two arrays are equal. --- Expected +++ Actual @@ @@ 4 => 'panels' - 5 => 'sections' - 6 => 'theme' - 7 => 'url' + 5 => 'previewableDevices' + 6 => 'sections' + 7 => 'theme' + 8 => 'url' )
#91
@
9 years ago
I'll be working on this today to resolve the outstanding issues and add whatever unit tests are missing.
#92
@
9 years ago
- Fixes the initial load issue by setting the default device when the Customizer is ready.
- Fixes the double iframe issue.
- Fixes a few coding standards and organization issues.
- Fixes the failing PHPUnit test.
- Adds a QUnit test to verify that
desktop
is initially set forapi.previewedDevice
. - Adds admin color support to the button borders.
- Typecasts the array in
customize.php
in the eventcustomize_previewable_devices
has been filtered incorrectly.
I believe the patch is ready for merge, it addresses the remaining issues and some that were not discussed. Great work everyone!
#93
@
9 years ago
Tested 31195.8.diff, and it's looking good.
@
9 years ago
Use 'default' as array key index instead of bare identifier since reserved word: https://github.com/xwp/wordpress-develop/commit/bdd51c2339bb66fbe94d5ca5a3b3ffcf5be2f0f4
This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.
9 years ago
#95
@
9 years ago
- Keywords commit added; needs-unit-tests removed
- Owner changed from valendesigns to westonruter
- Status changed from assigned to accepted
Proposed commit message:
Customize: Add a user-friendly way to preview site responsiveness for desktop, tablet, and mobile.
Introduces
WP_Customize_Manager::get_previewable_devices()
with acustomize_previewable_devices
filter to change the default device and which devices are available for previewing. This is a feature that was first pioneered on WordPress.com.
Props celloexpressions, folletto, valendesigns, westonruter, adamsilverstein, welcher.
Fixes #31195.
@celloexpressions: how much of the WordPress.com feature plugin informed your patches? @mattwiebe were you the author of this feature? Anyone else we should props?
#96
@
9 years ago
This looks really good. My only comment would be to make the hover and focus styles different. The simplest way to do that would be to remove the bottom border on the hover state. I mocked it up in 31195.10.diff
I also changed the transition so it only applies to the background. It looked kind of janky on the border because it is such a high contrast change.
#97
follow-up:
↓ 98
@
9 years ago
31195.10.diff and 31195.9.diff are looking good. I'm only curious why the word "also" was removed in 31195.2.diff?
#98
in reply to:
↑ 97
;
follow-up:
↓ 99
@
9 years ago
Is there a particular reason to make the hover and focus styles different? I'd be hesitant to make further changes to those at this point after the extensive back and forth that we already went through to agree on the current styles. That being said, I'd prefer to have the border for both and drop the background change from the focus styles. I'd also say that a higher contrast change would call for a longer transition, if anything, since no transition at all is more jarring.
I also just noticed - should we be running api.trigger
or api.preview.send
instead of only api.set
when the device value is changed? Not sure whether themes can hook into the value change binding from the preview.
Replying to ocean90:
31195.10.diff and 31195.9.diff are looking good. I'm only curious why the word "also" was removed in 31195.2.diff?
I was looking at something else, must've forgotten to remove it from the patch.
#99
in reply to:
↑ 98
@
9 years ago
Replying to celloexpressions:
Is there a particular reason to make the hover and focus styles different? I'd be hesitant to make further changes to those at this point after the extensive back and forth that we already went through to agree on the current styles. That being said, I'd prefer to have the border for both and drop the background change from the focus styles. I'd also say that a higher contrast change would call for a longer transition, if anything, since no transition at all is more jarring.
@celloexpressions an argument I see for letting the focus and hover styles be different is that if they are the same, then when you click on a different device there are two that have the same blue border. If the focus has a different style, then it is clear that there is one that is in an intermediate state (the one being clicked). I'll leave this up to you guys to finalize :)
I also just noticed - should we be running
api.trigger
orapi.preview.send
instead of onlyapi.set
when the device value is changed? Not sure whether themes can hook into the value change binding from the preview.
You mean instead of api.previewedDevice.set()
? No. There is no need to trigger
an event for this change because if any plugin is interested in the change to the previewedDevice
, they just need to add a listener on that Value
, e.g.:
wp.customize.previewedDevice.bind( function( newDevice ) { /*...*/ } );
Replying to ocean90:
31195.10.diff and 31195.9.diff are looking good. I'm only curious why the word "also" was removed in 31195.2.diff?
I was looking at something else, must've forgotten to remove it from the patch.
I restored “also” in 31195.11.diff.
Unless there are any objections, I'll go ahead and commit this and any other revisions can be committed subsequently as small iterative tweaks along with any other feedback after this gets into trunk
.
#101
@
9 years ago
Great to see this make it in!
@mattwiebe were you the author of this feature? Anyone else we should props?
It was primarily @matveb and @jacklenox, with contributions by @apeatling and myself.
Conceptual patch, could use refinement.