Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#42212 closed defect (bug) (fixed)

Customize Themes: Unstick open filters

Reported by: melchoyce's profile melchoyce Owned by: celloexpressions's profile celloexpressions
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.9
Component: Customize Keywords: needs-testing needs-patch
Focuses: ui Cc:

Description

When you open the theme filters, they "stick" so when you scroll, there amount of vertical space you have to view themes is significantly decreased. We should unstick the filters when open so that when you scroll, your theme isn't blocked by them. (The search bar/filter button container should still remain fixed.)

Attachments (8)

42212.patch (7.3 KB) - added by rclations 6 years ago.
42212.2.patch (7.8 KB) - added by rclations 6 years ago.
42212.3.patch (8.7 KB) - added by rclations 6 years ago.
42212.diff (7.5 KB) - added by obenland 6 years ago.
42212.2.diff (3.0 KB) - added by celloexpressions 6 years ago.
Relocate the conditional to load WordPress.org filters for the wporg section for improved extensibility and future compatibility.
42212.3.diff (2.5 KB) - added by celloexpressions 6 years ago.
Remove conditional stickiness.
IMG_0565.PNG (188.3 KB) - added by melchoyce 6 years ago.
IMG_0566.PNG (38.4 KB) - added by melchoyce 6 years ago.

Download all attachments as: .zip

Change History (36)

#1 @westonruter
6 years ago

  • Owner set to celloexpressions
  • Status changed from new to assigned

#2 @celloexpressions
6 years ago

The design intent here is to toggle the filters open and closed to switch between changing the filters and viewing themes. Note that nothing is sticky if there isn't room to view all of the filters vertically on your window size.

Maybe we should instead work on the styling of the feature filter toggle, particularly in the expanded state. Maybe adding a close icon could help?

I don't think that it would work to unstick when you scroll, because you couldn't scroll up in the themes list by doing this (past where you were when the filters opened). An exception is that changing the filters auto-scrolls to the top of the list (and pushes the themes grid down below the filters) - we could do that here, but would need to restick the filters when going back to the button. It's probably cleaner to keep the behavior simple and improve the interaction of the actual toggle.

@rclations
6 years ago

#3 @rclations
6 years ago

Added a patch that unticks the filters, allowing for scrolling to happen normally.

Also updated the JS toggling to facilitate easier access to the filters.

  • The filters are now only accessible at the top of the screen. Clicking the "Filter themes" button will automatically scroll you to the top of the page if you are not already there.
    • If you have opened the filters, but scrolled down the page, clicking the "Filter themes" button will leave the filters open, but scroll you up to them. Clicking a second time will close them.
    • If you have not opened the filters, clicking the button will scroll you up to the top and open the filters.

Also updated the positioning of the "Filter themes" button on mobile, since it looked a little out of place

Last edited 6 years ago by rclations (previous) (diff)

#4 @westonruter
6 years ago

  • Keywords has-patch added; needs-patch removed
  • Status changed from assigned to reviewing

@rclations
6 years ago

#5 @rclations
6 years ago

2 additions in the latest patch:

  • fixes offset for filter drawer on large screens
  • fixes unrelated issue w/ screens > 1670px wide, where the themes container was overlapping the left sidebar.
    • The left sidebar is set to 18%, with a min-width of 300px.
    • The themes container was set to (100% - 300px), which breaks once you reach 1670px and the left sidebar grows larger than it's min-width.

An additional note here: The theme container offset (used to make room for the filter drawer) is being set on load using js. Resizing the browser can break the display on the filter drawer as a result, with themes overlapping it.
See https://github.com/WordPress/WordPress/blob/master/wp-admin/js/customize-controls.js#L1812

Is this something we want to address?

#6 @rclations
6 years ago

Another day, another patch

  • Fixes offset for filter drawer on very small screens.
  • Updates js to calculate filter drawer size each time the drawer is opened, which allows for the display to work properly after resizing your browser window.
  • Updates filter group legend, increasing top padding to match bottom padding. This also updates the filter group display at /wp-admin/theme-install.php (screenshots below)

Before / After

https://cldup.com/C5g91l4JPd-2000x2000.png

Mobile display with updated legend styles

https://cldup.com/P2eQ1K6kqN-3000x3000.png

Last edited 6 years ago by rclations (previous) (diff)

@rclations
6 years ago

#7 @westonruter
6 years ago

  • Keywords needs-testing added

#8 @melchoyce
6 years ago

Okay, I was wrong about the spacing around layout/features/etc., we should probably revert those to what they were previously.

#9 @melchoyce
6 years ago

Otherwise though, let's merge!

@obenland
6 years ago

#10 follow-up: @obenland
6 years ago

@melchoyce I iterated on RC's patch and removed the top padding like you said. Let me know what you think?
It currently breaks if the browser window gets resized while the drawer is open (the height only gets recalculated when it's toggled). Not sure how much of an edge case that is and if you feel like that needs more attention.

#11 in reply to: ↑ 10 @melchoyce
6 years ago

Replying to obenland:

@melchoyce I iterated on RC's patch and removed the top padding like you said. Let me know what you think?

Looks good!

It currently breaks if the browser window gets resized while the drawer is open (the height only gets recalculated when it's toggled). Not sure how much of an edge case that is and if you feel like that needs more attention.

Yeah, would be good to fix, but it definitely is more of an edge case. I'm okay with leaving it as-is for now.

#12 @obenland
6 years ago

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

In 41903:

Customize: Unstick filter pane in Theme selector

Prevents the filter pane from blocking the view of theme search results when it's open.
Users are now able to just scroll to view results.

Props rclations.
Fixes #42212.

@celloexpressions
6 years ago

Relocate the conditional to load WordPress.org filters for the wporg section for improved extensibility and future compatibility.

#13 @celloexpressions
6 years ago

  • Keywords commit added; needs-testing removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

[41903] introduced a new method in WP_Customize_Themes_Section for rendering the filter drawer template. However, the call to that function was only made if it was the wporg instance. To make the distinct function useful for modular extensibility, the conditional should be located within the new method, and it should always be called. That way, custom themes sections can use custom filter bars and custom filter drawers by overriding those specific methods, without overriding the entire section template. If core introduces a filter drawer for installed themes (for multisite, for example), the new method can similarly be used without changing the entire template. This will also provide better future compatibility for any custom section types.

See 42212.2.diff.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

#15 @obenland
6 years ago

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

In 41973:

Customize: Ease extendability of filter drawer

By moving the wporg conditional inside the method body, the filter drawer can be overridden without having to also override the entire section template.

Props celloexpressions.
Closes #42212.

#16 @celloexpressions
6 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Version set to trunk

Following [41903], the filter bar should always be sticky, regardless of screen size. Previously, it was only sticky if the sticky filter bar would fit on the screen when toggled. It is now possible to always stick the filter bar because the filter drawer is never sticky, and will be able to fit regardless of the screen size. The UX is very odd with it unsticking at many screen sizes now (including mobile, where there is no easy way to scroll to the top).

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


6 years ago

@celloexpressions
6 years ago

Remove conditional stickiness.

#18 @celloexpressions
6 years ago

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

42212.3.diff is a first pass at removing the conditional stickiness. Could use more eyes on the JS in particular, based on how that was intended to work. Some magic numbers in here currently.

#19 @westonruter
6 years ago

@melchoyce I've deployed this change to a test env for review: http://trac-39896-xwp-testbed.pantheonsite.io/wp-admin/customize.php

@melchoyce
6 years ago

@melchoyce
6 years ago

#20 @melchoyce
6 years ago

Pretty broken on my phone. See attached screenshots.

#21 @melchoyce
6 years ago

@celloexpressions Can you add screenshots or a video of how you're seeing it work now, vs. your patch's intended results? Hard to understand your intention without a visual.

This ticket was mentioned in Slack in #core-customize by celloexpressions. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

#24 @westonruter
6 years ago

  • Milestone changed from 4.9 to 4.9.1

I'm also noticing issues with getting the theme filters pane to toggle on or off. The list of checkboxes seems to be hidden sometimes when expanding the list. If I hit it repeatedly then the filter list can appear behind the list of themes.

I'm going to punt this as it's not likely we'll have a tested updated patch before RC1.

#25 @celloexpressions
6 years ago

The version in 4.9RC1 is still broken - the filter bar disappears on screens shorter than a magic number (or narrower than a different, also magic number), unless you scroll all the way up to the top (quite difficult with one-column infinite scroll of themes). I don't happen to use this feature on any devices that would be affected by this problem, but the experience is quite subpar for those users currently. I'm guessing there might be some issues with CSS caching in the above screenshots, or, those that worked on the previous changes in this ticket may have a better idea of why certain browsers don't like something here. Probably worth exploring before 4.9 release if anyone is able to do that, though.

#26 @westonruter
6 years ago

  • Keywords needs-patch added; has-patch removed

#27 @johnbillion
6 years ago

  • Milestone changed from 4.9.1 to 4.9.2

#28 @westonruter
6 years ago

  • Milestone changed from 4.9.2 to 4.9
  • Resolution set to fixed
  • Status changed from reopened to closed

Remaining issues to be followed up in #42872.

Note: See TracTickets for help on using tickets.