Skip to content
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] Design Review Check #2563

Closed
chipsnyder opened this issue Aug 20, 2020 · 32 comments
Closed

[Modal Layout Picker] Design Review Check #2563

chipsnyder opened this issue Aug 20, 2020 · 32 comments
Assignees
Labels
Page Templates [Type] Enhancement Improves a current area of the editor

Comments

@chipsnyder
Copy link
Contributor

Is your feature request related to a problem? Please describe.

This task is part of the effort to rework the Starter Page Template Picker for new pages. This part of the initiative will focus on modifying the API call /wpcom/v2/sites/{site}/block-layouts to include the supported_blocks
query parameter which will be used to only include layouts that have met a server defined threshold of supported blocks.

Goals

  • This issue is to simply serve as a reminder to wrap up and discuss any lingering design review elements that come up in the course of developing the Modal Layout Picker.

Additional context

Blueprint Flow - iOS
Blueprint Flow - Android
@chipsnyder chipsnyder added [Type] Enhancement Improves a current area of the editor Page Templates labels Aug 20, 2020
@chipsnyder
Copy link
Contributor Author

On iOS things that we still need to figure out:

  1. Bottom Buttons in Dark Mode
    • In Dark mode, the style of the border on the buttons at the bottom is a bit difficult to see.

    • Example
      iOS 13+ Dark Mode
  1. When selecting a layout, it would be nice to add an animation for their presentation
  2. Review the animation of cells for when the layouts have been fetched but an image hasn't downloaded yet

cc @iamthomasbishop

@iamthomasbishop
Copy link
Contributor

@chipsnyder Thank you for the updated builds and for making this ticket for easier review. I'm checking out the latest build in the Category bar PR over here and will drop my notes below:

  • When I select a single filter while the category bar is collapsed, the scroll-position jumps to the top which shows the full header. Is it possible to maintain the collapsed state on the category bar when this happens? It's fine if the canvas scrolls to the top of the scroll area, but re-expanding the category bar feels a little awkward. (to reproduce: scroll down a few sections, then tap the "Blog" filter chip and you'll notice the canvas not only scrolls to top, but also expands the header)

  • There is sometimes a bit of a momentary lag when you're scrolling quickly -- I'm not sure if this is a performance thing due to resource loading or something w/ the way we're handling various element states while scrolling. I'll try on other devices tomorrow to see if I can reproduce.

  • As you mentioned above, it would be great if we can get a quick subtle transition for the selected/un-selected states of the image thumbnail cells. A quick 100ms transition would probably feel pretty good as a starting point. Bonus points if we can animate the stroke along the path of the checkmark icon, but definitely not a blocker. 😄

  • Is it possible to remove the slight tint that is standard on the system materials we're using on the header? Ideally, I'd like it to be the same as the main sheet's surface background color. Also not a deal-breaker, but would be preferred.

I will come back to investigate what we might do regarding the outlined buttons in dark mode tomorrow. I think we may have other instances in the app where we may experience the same problem/have a similar solution.

@chipsnyder
Copy link
Contributor Author

Hey @iamthomasbishop

  • When I select a single filter while the category bar is collapsed, the scroll-position jumps to the top which shows the full header. Is it possible to maintain the collapsed state on the category bar when this happens? It's fine if the canvas scrolls to the top of the scroll area, but re-expanding the category bar feels a little awkward. (to reproduce: scroll down a few sections, then tap the "Blog" filter chip and you'll notice the canvas not only scrolls to top, but also expands the header)

Yeah, we can explore this flow. Before I get started on it, let me run a use case by you.

  1. Open MLP
  2. (Don't scroll vertically) Select a category.

Should the header stay open or should it collapse?

The other case I want to get your thoughts on would be:

  1. Open MLP
  2. Scroll Down a few sections,
  3. Then tap the "Blog"
    • (After the code change this would stay collapsed)
  4. Try to scroll
    Should the header stay collapsed or snap open? IMO here it would stay collapsed otherwise it would probably feel weird that it opened from what could be slight touch.
  • There is sometimes a bit of a momentary lag when you're scrolling quickly -- I'm not sure if this is a performance thing due to resource loading or something w/ the way we're handling various element states while scrolling. I'll try on other devices tomorrow to see if I can reproduce.

Let me know on this one. I haven't really experienced it yet but the build to fetch live data and the later ones to optimize that data might help. I'll keep an eye out.

I'll take a look at the other two and get back to you on those. In the meantime, the most up to date build can be found here: wordpress-mobile/WordPress-iOS#14750 (comment)

It has:

  1. Fetching real data (but not all of the optimizations yet)
  2. You can now Create a Page from a selected layout

@iamthomasbishop
Copy link
Contributor

Open MLP
(Don't scroll vertically) Select a category.

Should the header stay open or should it collapse?

It should stay in its current state, so in that case it'd stay open.

Should the header stay collapsed or snap open? IMO here it would stay collapsed otherwise it would probably feel weird that it opened from what could be slight touch.

Agreed, it should stay collapsed (stay in its current state, per previous comment).

Let me know on this one. I haven't really experienced it yet but the build to fetch live data and the later ones to optimize that data might help. I'll keep an eye out.

I think it might only be happening on first load and it's not super noticeable, just a little stutter here and there if I'm scrolling as the images are loading, and it's less noticeable on iPad -- so it's probably a performance thing that would be more noticeable on older/slower devices.

the most up to date build can be found here

I'm not seeing a build available on that, but I'll keep an eye out.

@chipsnyder
Copy link
Contributor Author

I'm not seeing a build available on that, but I'll keep an eye out.

😅 Oops I thought I triggered that build but I just had it open and never hit start.

Sounds good on the other comments. That should be pretty manageable then. I'll get started on that and let you know if I have any other Q's

@iamthomasbishop
Copy link
Contributor

Ah okay, just got the new build, it's great to see real data in there! I'll keep an eye out for updates that need design review 😄

@iamthomasbishop
Copy link
Contributor

@chipsnyder I'm seeing another issue that I saw during the last round of reviews but I thought was my imagination 😆. When the modal sheet is opening and is loading, it looks like some of the colors (header background and some placeholder items like the category chips and a layout thumbnail placeholder) are a lighter color than desired, resulting in what can feel like a "flicker". Here's what I'm seeing, on both iPhone 11 Pro and iPad Pro: video, image

Note: I'm on iOS 14, so some of this could potentially be iOS 14-specific shenanigans, but I haven't seen it on any other apps afaik.

@chipsnyder
Copy link
Contributor Author

When the modal sheet is opening and is loading, it looks like some of the colors (header background and some placeholder items like the category chips and a layout thumbnail placeholder) are a lighter color than desired, resulting in what can feel like a "flicker"

In the fetching live data PR I added a ghost of a single layout and a single category (with filter button) I just failed to check the dark mode setting for that. I'll adjust that and see what we're using for other ghosts in dark mode.

@iamthomasbishop
Copy link
Contributor

In the fetching live data PR I added a ghost of a single layout and a single category (with filter button) I just failed to check the dark mode setting for that. I'll adjust that and see what we're using for other ghosts in dark mode.

Ahh, that makes sense. Thank you!

@antonis
Copy link

antonis commented Sep 22, 2020

Hello @iamthomasbishop 👋 ,
Apologies for the delayed reply on your design review. I'll move this thread here to have a central point of reference on the Android design feedback.

You can download the latest build from here (PR).

  • I noticed when a row of thumbnails rests below the surface containing the category chips/buttons, I'm unable to interact with or even scroll the categories container -- instead, the thumbnails row is scrolled

This is fixed in the latest build. This actually required some refactoring that caused delay.

  • Would it be possible to style the sheet in its full-height state with rounded corners, and to hold the same scrim/backdrop style that it has when it's not fully docked? Here's a visual for example.

I'm working to achieve this in a separate PR. It seems that there are some restrictions on this in material components but there are also workarounds to overcome them.
Note: This build has only flat corners and the background has fallen back to the default after merging from dev.

  • Can we by any chance replicate the same multi-select capabilities that we're rolling with on iOS?

Fixed

  • I'm seeing the large title disappear every once in a while, especially when interacting with the category chips.

Fixed

  • The thumbnail rows have a bit too much padding on the bottom, but it sounds like you've sorted that out above.

Fixed

  • Would it be possible to apply a smooth transition to the style changes between an un-selected and selected thumbnail image? I believe Material's recommendation for such a transition is an easeOut transition with a 100ms duration, but if we're already using transitions elsewhere natively, it might make more sense to use something that exists.

This should have a default transition. Let me know if this should change.

  • Can we use the standard elevation shadow on the app bar, even when the category bar is "fixed"

Fixed

  • Can we use Noto Serif (bold) for headings?

Fixed

  • What font-size is the compact app bar using (when it is "fixed" on scroll)? It feels slightly large, but it might just be my eyes playing tricks, or because I'm used to seeing titles left-aligned on the 72dp keyline. (note: I believe 20sp is the standard size)

It is set to textAppearanceHeadline6 that is 20sp. Let me know if that should be changed.

Thanks for all of the great work on this thus far! 👏

Thanks again you for reviewing this and sorry for the delayed response 🙇

@iamthomasbishop
Copy link
Contributor

@antonis thanks for the update, this is looking really solid.

I'm working to achieve this in a separate PR. It seems that there are some restrictions on this in material components but there are also workarounds to overcome them.

Sounds good 👍

This should have a default transition. Let me know if this should change.

Transition looks nice, thank you!

It is set to textAppearanceHeadline6 that is 20sp. Let me know if that should be changed.

This should be fine. 20sp is correct, my eyes must have been playing tricks on me 😀

As I was going through the latest build, I also jotted down a few notes:

  • Can we change the background color of the StatusBar (when the sheet is fully open) to white? (Or, alternatively, a very weak gray?) (example)
  • Can we nudge the back arrow to the left slightly so that it aligns with the 20dp keyline? (example)
  • It looks like there is a background fill behind the shadow on the footer bar (example)
  • Can we apply a background fill behind the checkmark icon? Currently, because the check part of it doesn’t have a fill, it can get lost a little bit when it sits over anything other than a white surface (example)
  • I think the margins/sizings on the top app bar (when collapsed) are a bit off, and maybe this was one of the reasons the label looked larger 😊. I’m not 100% sure how you have the elements laid out, but it looks like the drag handle sits in its own area (12dp tall?), whereas the original mockups had it integrated into a standard 56dp-tall App Bar. (example). Another note: I’d like to reduce the spacing below the Chips in the collapsed App Bar to 8dp instead of 16dp (see example above). (Note: alternatively, if we have to keep the separate space for the drag handle, let’s just add it above the App Bar as shown in the last example above)

I think that’s all I’ve got right now! It’s looking pretty great, so nice work! 👏

@antonis
Copy link

antonis commented Sep 23, 2020

Hello @iamthomasbishop 👋 ,

Thank you for the prompt review. I really appreciate it 🙇

You can download the latest build from here.

  • Can we change the background color of the StatusBar (when the sheet is fully open) to white? (Or, alternatively, a very weak gray?) (example)

I'll try to cover this along the the rounded corners bottom sheet implementation

  • Can we nudge the back arrow to the left slightly so that it aligns with the 20dp keyline? (example)

Fixed

  • It looks like there is a background fill behind the shadow on the footer bar (example)

Fixed

  • Can we apply a background fill behind the checkmark icon? Currently, because the check part of it doesn’t have a fill, it can get lost a little bit when it sits over anything other than a white surface (example)

Fixed

  • I think the margins/sizings on the top app bar (when collapsed) are a bit off, and maybe this was one of the reasons the label looked larger 😊. I’m not 100% sure how you have the elements laid out, but it looks like the drag handle sits in its own area (12dp tall?), whereas the original mockups had it integrated into a standard 56dp-tall App Bar. (example). Another note: I’d like to reduce the spacing below the Chips in the collapsed App Bar to 8dp instead of 16dp (see example above). (Note: alternatively, if we have to keep the separate space for the drag handle, let’s just add it above the App Bar as shown in the last example above)

Fixed

@antonis
Copy link

antonis commented Sep 23, 2020

Hello @iamthomasbishop 👋 ,

  • Can we change the background color of the StatusBar (when the sheet is fully open) to white? (Or, alternatively, a very weak gray?) (example)

regarding the StatusBar color it seems hard to set it only in expanded state. Actually the StatusBar color is already white and the BottomSheet has a dim background. If I remove this background I get the white bar color but I loose the background dim completely (only a top shadow is left). Wdyt of this option?

You can find a build for testing here and some screenshots bellow.

Expanded state Collapsed state 1 Collapsed state 2
device-2020-09-23-234213 device-2020-09-23-234748 device-2020-09-23-234734

@iamthomasbishop
Copy link
Contributor

iamthomasbishop commented Sep 23, 2020

@antonis I like that a lot, it looks and feels natural. I think we can probably also remove the rounded corners as well because there can be a moment when the sheet is just shy of fully expanded, where the shadow shines through the gap around the corners (screenshot).

I was also inspecting the header area a bit more closely because something felt off to me, and noticed a couple of things:

  • The drag handle looks off-center by about ~8dp (example)
  • I can't believe I didn't think of this earlier, but we should use sentence casing on the "Choose a layout" heading (only on Android) 🤦
  • The spacing in the header area feels a little bit inconsistent, is there any way we can adjust the spacing? I'm not sure what the current values are, but if possible, maybe we can adjust as follows (visual example):
    • Top bar <=> big headline: +10dp
    • Big headline <=> sub-headline: -22dp
    • Sub-headline <=> chips: -5dp

That's all I've got for now, this feels really close!

@antonis
Copy link

antonis commented Sep 24, 2020

Hello @iamthomasbishop 👋 ,

Thank you once more for the feedback 🙇

You can download the latest build from here ⬇️

I think we can probably also remove the rounded corners as well because there can be a moment when the sheet is just shy of fully expanded, where the shadow shines through the gap around the corners (screenshot).

Fixed
  • The drag handle looks off-center by about ~8dp (example)

Fixed

  • I can't believe I didn't think of this earlier, but we should use sentence casing on the "Choose a layout" heading (only on Android) 🤦

Fixed

  • The spacing in the header area feels a little bit inconsistent, is there any way we can adjust the spacing? I'm not sure what the current values are, but if possible, maybe we can adjust as follows (visual example):

    • Top bar <=> big headline: +10dp
    • Big headline <=> sub-headline: -22dp
    • Sub-headline <=> chips: -5dp
Fixed

@iamthomasbishop
Copy link
Contributor

@antonis This is looking fantastic. I have a couple of tiny tiny requests, and forgive me for being a perfectionist here but we're so close we might as well 🤷 😆

  • The spacing between the top bar and big headline is ever so slightly larger than I'd like (I think in my last comment I proposed 20dp, whereas in the latest build it's 26dp). Rather than going down to the 20dp that I requested previously, let's just round down to the next logical grid-line, 24dp (so -2dp from the current value). Here's a visual for reference:

  • Is it just me or is the "y" in the word "layout" cut off at the bottom of the letterform?

@antonis
Copy link

antonis commented Sep 24, 2020

Hello @iamthomasbishop 👋

@antonis This is looking fantastic. I have a couple of tiny tiny requests, and forgive me for being a perfectionist here but we're so close we might as well 🤷 😆

Thanks again for your detailed feedback and your patience :)
I hope I got it right this time 🤞

  • Is it just me or is the "y" in the word "layout" cut off at the bottom of the letterform?

I must be blind. This was even in my screenshots and didn't see it 🙈

The new build can be downloaded from here ⬇️

Screenshot with keylines

@chipsnyder
Copy link
Contributor Author

Hey @iamthomasbishop, I put together a PR build for you to test out with updates to the design items. I included everything I fixed in the description and I think the tasks for caching data took care of some of the laggy-ness you mentioned. I'd like to use that to address any lingering design items you have so feel free to drop your notes here or there whichever is easier :)

@iamthomasbishop
Copy link
Contributor

iamthomasbishop commented Oct 5, 2020

@chipsnyder Thanks for the updated build! This is looking pretty sharp. The surface colors and transitions look solid, and I’m not seeing any issues with scrolling or selections. The only thing that feels off to me is the transitions on the buttons in the footer toolbar when selecting/deselecting layouts. Here’s a video of what I’m referring to. Is there any way we can just cross-fade that transition?

@antonis — I forgot to get back to you on the Android review, but it's looking super sharp! I don't have any issues with shipping it. Thanks for your patience! 🙏

@chipsnyder
Copy link
Contributor Author

Thanks @iamthomasbishop!

Is there any way we can just cross-fade that transition?

Yeah that shouldn't be a problem.

@chipsnyder
Copy link
Contributor Author

@iamthomasbishop The design review PR is updated with that fade change now.

@antonis
Copy link

antonis commented Oct 8, 2020

Hello @iamthomasbishop 👋 ,

You can find one more Android build here ⬇️

This is still under review code-wise, but I thought it would be good to have an early look since it adds the Preview screen. This is based on the Editor screen removing the top toolbar buttons and adding the bottom toolbar.
Regarding the UI the only feature missing from the final product is the toast messages after creating a new page.

Thanks again for your feedback on this project 🙇

@antonis
Copy link

antonis commented Oct 9, 2020

Hello @iamthomasbishop 👋

You can find one more build here ⬇️

This includes the toast messages I mentioned above.
Though this is still under review the only think missing is the caching mechanism that should make the UI more responsive in some cases.

Thanks again for your help on this 🙇

@iamthomasbishop
Copy link
Contributor

@antonis this looks great overall, super close! I think we're good in terms of spacing/sizing and other design details from the previous reviews, I just found 2 things worth noting:

  • I'm seeing this RN error when applying the Seedlet home page layout. This looks related to the Media & Text block, and while you can escape the error and proceed to create the page, the error will appears again upon editor load. (screenshot)
  • I'm not sure if this is something new, but the point at which the large title hides and becomes the smaller app bar title looks a little bit jarring if you're not scrolling quickly. Notice in this example video where I'm swiping to go down the page, the transition happens when about half of the large label's height is hidden. The result is what feels like the title "jumping" ~20-40dp. To make this feel more natural, I think we might want to wait until it's almost completely hidden behind the app bar surface. I also think a subtle cross-fade animation might help to make it less jarring, fwiw.

@antonis
Copy link

antonis commented Oct 15, 2020

Hello @iamthomasbishop 👋 ,

thank you for your feedback on this 🙇

  • I'm seeing this RN error when applying the Seedlet home page layout. This looks related to the Media & Text block, and while you can escape the error and proceed to create the page, the error will appears again upon editor load. (screenshot)

Nice catch 👍 This is related with Media & Text block failing (it happens on iOS too, but without a crash cc @chip).
We'll need to investigate this deeper on the Gutenberg side.

  • I'm not sure if this is something new, but the point at which the large title hides and becomes the smaller app bar title looks a little bit jarring if you're not scrolling quickly. Notice in this example video where I'm swiping to go down the page, the transition happens when about half of the large label's height is hidden. The result is what feels like the title "jumping" ~20-40dp. To make this feel more natural, I think we might want to wait until it's almost completely hidden behind the app bar surface. I also think a subtle cross-fade animation might help to make it less jarring, fwiw.

I changed the scroll threshold and added a cross-fade animation.

You can find the new build here ⬇️

@iamthomasbishop
Copy link
Contributor

@antonis The transition is much better, thank you! 👏

This is related with Media & Text block failing
We'll need to investigate this deeper on the Gutenberg side.

Ok, sounds good.

@antonis
Copy link

antonis commented Oct 16, 2020

Hello @iamthomasbishop 👋 ,
Thank you for all your help on MLP development 🙇
At this point the feature is wrapped up on Android too 🎉

We discussed a minor enhancement with @malinajirka regarding the ability to retry loading the layouts when an error occurs and there are no cached data to show. This is actually the default error/retry view as seen in the screenshot here.

The user is still able to proceed with creating an empty page even in offline mode but also has the option to retry fetching the layouts.

Do you have any feedback on this?

@iamthomasbishop
Copy link
Contributor

Do you have any feedback on this?

@antonis It looks okay, but I think there's room for refinement.

  • I think we should "minimize" the large title/description in this scenario, because it draws the eye away from the available actions (specifically the footer action)
  • The generic "no network available" sounds a bit mechanical imo
  • The footer action feels somewhat detached in this context, so I wonder if we should group it with the retry button in some way

With those things in mind, here are a few quick suggestions for refinement. Let me know if you have any preferences or feedback.

@antonis
Copy link

antonis commented Oct 20, 2020

Hello @iamthomasbishop 👋 ,
thank you once again for the detailed feedback👍 I really appreciate it 🙇

I implemented Proposal D. Please check the screenshot below or the build from the PR.

@chipsnyder
Copy link
Contributor Author

This is a good idea. I'll take a look at adding something like this on iOS as well. I think we have a standard no results controller. So I'll start with that and share the results in a PR

@iamthomasbishop
Copy link
Contributor

Looks great @antonis. My one worry was whether moving the retry button position would cause any issues wrt to the other empty/retry states, but I don't foresee it being a big issue.

// cc @malinajirka do you see any issues w/ moving the retry action out of the center area into the footer? ☝️

@antonis
Copy link

antonis commented Oct 20, 2020

Thank you for the feedback @iamthomasbishop :)

My one worry was whether moving the retry button position would cause any issues wrt to the other empty/retry states, but I don't foresee it being a big issue.

This change does not change the reusable empty/retry view and does not affect any other empty/retry state

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Page Templates [Type] Enhancement Improves a current area of the editor
Projects
None yet
Development

No branches or pull requests

4 participants
@antonis @iamthomasbishop @chipsnyder and others