-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Modal Layout Picker] Create Layout Picker category bar UI using static data #14631
Conversation
…ssue/2437-categoryBar
…ssue/2437-categoryBar
…ssue/2437-categoryBar
…ssue/2437-categoryBar
…ssue/2437-categoryBar
…ssue/2437-categoryBar
…ssue/2437-categoryBar
You can trigger optional UI/connected tests for these changes by visiting CircleCI here. |
…ssue/2437-categoryBar
WordPress/Classes/ViewRelated/Gutenberg/Layout Picker/GutenbergLayoutFilterBar.swift
Outdated
Show resolved
Hide resolved
@@ -27,7 +27,7 @@ class GutenbergLayoutPickerViewController: UIViewController { | |||
@IBOutlet weak var titleView: UILabel! | |||
@IBOutlet weak var largeTitleView: UILabel! | |||
@IBOutlet weak var promptView: UILabel! | |||
@IBOutlet weak var categoryBar: UICollectionView! | |||
@IBOutlet weak var filterBar: GutenbergLayoutFilterBar! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed this just to support a more generic naming structure as this might be used for Gutenboarding later.
@@ -317,23 +322,24 @@ extension GutenbergLayoutPickerViewController: UITableViewDelegate { | |||
} | |||
|
|||
private func containsSelectedLayout(_ layout: GutenbergSelectedLayout, atIndexPath indexPath: IndexPath) -> Bool { | |||
let rowSection = sections[indexPath.row] | |||
let rowSection = (filteredSections ?? sections)[indexPath.row] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about creating a getter for (filteredSections ?? sections)
but I felt like that was clouding when I was explicitly trying to reference filteredSections
or sections
vs when I wanted the one that was appropriate for the current display mode
Hey @guarani 👋 I was wondering if you'd be able to help me review this piece of the Modal Layout Picker work? I wanted to try and spread out the code reviews across the team (: |
Sure thing @chipsnyder :) I'll try to get to this asap. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and looking great! 💯
I tested all the scenarios on a iPhone 11 with iOS 14 in light/dark mode and in portrait/landscape modes, then did a quick check with an iPhone SE and iPad simulators. The only issues I found look minor and/or opinionated — and some are UX related — so feel free to merge this as-is:
-
I think the template cards would benefit from a snap-to-center behavior: At the moment you can select a template that's partially (or almost fully) off-screen, and it doesn't scroll into view or snap to center.
-
As the user selects categories one-by-one, would it be useful to scroll that most recently selected category into view? This happens naturally for the first category selected (since all others are explicitly hidden), but as more categories are selected, they are added off-screen which gives the user no visual feedback for their action. What I'm not sure about is whether anything could be done as categories are deselected.
-
Since the user can select a template, then filter out the category that template belongs to, I'm not sure if we should still enable the Create Page button — since it will create a layout based on a selection that's not currently visible. (Based on the PR description, this seems to be by design, but it seems awkward to me.)
-
I noticed the templates can take a while to load, not sure if a loading view will be needed here.
-
On iPads, I noticed that the category filter buttons are left-centered (as a group) — not sure if by design or if this should be centered like the title above it.
-
On iOS versions below 13, the category button selected color is off (also, deselecting the button doesn't return it to its original visual state)
-
The "Create Blank Page" and "Preview" buttons use a think greyish border that works well in light mode, but in dark mode can be a bit hard to distinguish in certain situations. When you scroll the list of templates so that a white template is under these buttons, the button border is hard to make out.
WordPress/Classes/ViewRelated/Gutenberg/Layout Picker/GutenbergLayoutFilterBar.swift
Outdated
Show resolved
Hide resolved
WordPress/Classes/ViewRelated/Gutenberg/Layout Picker/GutenbergLayoutPickerViewController.swift
Outdated
Show resolved
Hide resolved
...Press/Classes/ViewRelated/Gutenberg/Layout Picker/LayoutPickerFilterCollectionViewCell.swift
Outdated
Show resolved
Hide resolved
...Press/Classes/ViewRelated/Gutenberg/Layout Picker/LayoutPickerFilterCollectionViewCell.swift
Outdated
Show resolved
Hide resolved
...Press/Classes/ViewRelated/Gutenberg/Layout Picker/LayoutPickerFilterCollectionViewCell.swift
Show resolved
Hide resolved
...Press/Classes/ViewRelated/Gutenberg/Layout Picker/LayoutPickerFilterCollectionViewCell.swift
Outdated
Show resolved
Hide resolved
...ess/Classes/ViewRelated/Gutenberg/Layout Picker/LayoutPickerCategoryCollectionViewCell.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved, as mention in above comment all issues are minor.
Thanks for the review @guarani!
I like this idea I'll give that a try. :)
@iamthomasbishop Do you have any thoughts on this?
Yeah, this is by design, @iamthomasbishop has a response here (wordpress-mobile/WordPress-Android#12573 (comment)) that can shine more light on what he's thinking from the UX point of view.
This is actually being taken on as part of the fetching live data portion which I separated out to another task. #14699. The thumbnail images will also be enhanced as a future portion as well.
These are great catches I'll investigate these a bit more before merging and report back 👍
Yeah I totally agree, @iamthomasbishop and I are exploring some other options for this. |
Hey @guarani, I took care of the color issues and layout issues you mentioned. Also:
I gave this a try it felt a bit weird to me to have it shift. I tried a few apps (mostly table views instead of collection views) and when making a selection while remaining on the same page the selected item didn't move. @iamthomasbishop @guarani If you want I can put a build together in another branch so we can try it if you'd like. Let me know if you'd like to take another look. If not I'll move forward with this and handle the remaining design items in a future branch. |
Thanks for looking into it @chipsnyder, but not to worry, if it's worthwhile maybe it can be explored in the future from a design perspective. |
Fixes wordpress-mobile/gutenberg-mobile#2437
To test:
To disable or enable the development version of Modal Layout Picker
When enabled, the Layout Picker should show when creating a new page from My Site and from the Page list.
Start by enabling the MLP (if needed) and by navigating to the Modal Layout Picker
Select a Category
Select Multiple-Categories
Selecting a Layout then filter on the same category
Selecting a Layout then filter on the same category deselect layout
Selecting a Layout then filter on a different category
Selecting a Layout then filter on a different category select a different layout
Screenshots:
Additional Context:
This is a continuation of the work in #14553 and will be followed up by other tickets as part of Modal Layout Picker Project
PR submission checklist:
RELEASE-NOTES.txt
if necessary.