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

Feature/quick actions v1 #10362

Merged
merged 20 commits into from Aug 22, 2019

Conversation

@khaykov
Copy link
Member

commented Aug 8, 2019

This PR adds the first iteration of static Quick Actions.

PR looks a bit big, but the majority of changes is from moving things around.

To test:

  • Make sure quick action buttons work ok.
  • Quick Start, pointers point to the correct rows and blavatar.
  • Tracking events are getting hit.
  • Nothing looks weird.
  • Clicking on Site Name or URL opens site preview.
  • Users without access to pages should not see Pages quick action.

63117921-9b48b380-bf51-11e9-9eb4-14dcb514ade1

Update release notes:

  • If there are user facing changes, I have added an item to RELEASE-NOTES.txt.
@osullivanchris

This comment has been minimized.

Copy link

commented Aug 13, 2019

Nice work on this. I'm just back from AFK and working through a redline for you @khaykov. I'll have it to you in a couple hours and call out any major differences.

@osullivanchris
Copy link

left a comment

Hey I have red-lined the original mocks so that we can nail all the details. There are some small things I can see which are slightly off. E.g. site icon, text sizes and colours, size of Quick Action buttons, spacings. I know you were working without a spec so I think you should have everything you need here now and hopefully its just a case of changing values and not too much work. The text sizes are taken from those defined here https://material.io/design/typography/the-type-system.html#type-scale and the colours are taken from our colour studio values.

One thing I noticed that I am not clear on how to spec is the shadows on the Quick Actions. I'm not sure how we define shadows today but I found these guidelines which specify default elevation values.

https://material.io/design/environment/elevation.html#default-elevations

It looks like you have something like a FAB (value 6dp) right now. I think we need something more like a card (1dp) or button (2dp)

If we don't use those elevation values, and need to code it ourselves for now, the values in the sketch file are 1px distance, 2px blur, HEX 000000 with 20% opacity (alpha 20)

Let me know if any more clarification needed or any other questions!

specs

@khaykov

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

@osullivanchris Thanks for the redlines and comments!

I was using FAB for quick action buttons, so yes, they had a big shadow. I changed it to card shadow that we are using (2dp).
Their dimensions are OS default, so I haven't touched them.

I changed the font type of site title and switch site button to medium and removed bold.

Hope I didn't miss anything!

Also, I was wondering, is there anything extra you maybe want to track?

Screenshot_1565760536

@osullivanchris

This comment has been minimized.

Copy link

commented Aug 14, 2019

@khaykov looks much better thanks for making those tweaks!

Just a few more to follow up on:

  1. Size of the Quick Action buttons

Their dimensions are OS default, so I haven't touched them.

Sorry the default dimension for what? A FAB size or something else? I thought these circle buttons were our own design rather than a default component, so I would have gone with the size per the mock. But maybe there's something I'm missing.

  1. A few tweaks to the style of site icon
  • In the screenshot of the new build, the overall area of the site image with the border seems to be right. But the border looks to be 8dp rather than 4dp. Could you make the border narrower but keep that overall size as is? Correct size is 64dp image, 4dp borders all round making 72dp total (which you have currently just with too much border)
  • Rather than a shadow there is a solid line border and no shadow in the mock (value can be seen in the red line)
  • Something I didn't call-out in the red line that I now notice is different is that there is a border radius around the image as well as the outer shape. The internal radius is 2dp and the external is 4dp

Screenshot 2019-08-14 at 11 34 12

Might not be exactly how you've written it in code but hope that makes sense! Thanks!

@khaykov

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2019

Sorry the default dimension for what? A FAB size or something else? I thought these circle buttons were our own design rather than a default component, so I would have gone with the size per the mock. But maybe there's something I'm missing.

The dimension of FAB. Quick action buttons are just FAB's, so they already have correct dimensions.

In the screenshot of the new build, the overall area of the site image with the border seems to be right. But the border looks to be 8dp rather than 4dp. Could you make the border narrower but keep that overall size as is? Correct size is 64dp image, 4dp borders all round making 72dp total (which you have currently just with too much border)

Good catch! Totally missed that one.

Rather than a shadow there is a solid line border and no shadow in the mock (value can be seen in the red line)

👍

Something I didn't call-out in the red line that I now notice is different is that there is a border radius around the image as well as the outer shape. The internal radius is 2dp and the external is 4dp

👍

I think it looks about right now :)
Screenshot_1565824213

@osullivanchris

This comment has been minimized.

Copy link

commented Aug 15, 2019

@khaykov Yup looking great now. Checked with @SylvesterWilmott and yes the Quick Actions should be the same size as FABs. When I'm measuring it here it looks like the buttons are 48dp when I would expect 56. But it might be that I'm looking at the wrong density. Per the guidelines the only sizes are 56 and 40. Assuming you're using 56 already as 40 would be much too small.
Screenshot 2019-08-15 at 14 37 29

The guidelines also mention only having one FAB per page. As long as it looks right I'm ok. And it looks right (just double checking that detail above). Just wanted to mention this in case there is any semantic issues or incase we get dinged in app store review or elsewhere. As I said once it looks like the mock I'm happy!
Screenshot 2019-08-15 at 14 38 52

Two other small details that came up chatting to Sylvester

  • Can we make the quick start card the same style as the stats card (full-width and no shadow). It was designed to match the card above which we have now killed, so it looks a bit off having one thing as a card. We can make a separate task for that if you want
  • Can you add the white background to the 'Register domain' row so it matches the other rows below it?
@khaykov

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2019

@osullivanchris Thanks for the guidance! I updated layout:

ab

The guidelines also mention only having one FAB per page. As long as it looks right I'm ok. And it looks right (just double checking that detail above). Just wanted to mention this in case there is any semantic issues or incase we get dinged in app store review or elsewhere. As I said once it looks like the mock I'm happy!

I double-checked and the size is correct 👍Also, it's FAB only in code, for all other purposes it's just a round button.

@osullivanchris

This comment has been minimized.

Copy link

commented Aug 15, 2019

All looks great @khaykov thanks for going through those extra details!

@peril-wordpress-mobile

This comment has been minimized.

Copy link

commented Aug 21, 2019

Warnings
⚠️ PR has more than 500 lines of code changing. Consider splitting into smaller PRs if possible.

Generated by 🚫 dangerJS

khaykov added 2 commits Aug 21, 2019
@theck13
Copy link
Contributor

left a comment

I left some change requests in the code for review. I also have a few comments about the interface. We may want to summon @osullivanchris for input on the following.

The touch feedback for the Switch Site button isn't what I would expect from a text button. Material guidelines show a rectangular background when pressing text buttons. Should we change the background from ?attr/selectableItemBackgroundBorderless to ?attr/selectableItemBackground and mimic the background border of the Change Photo text button on the Me tab? See the screenshots below for illustration.

10362_button

The Quick Action buttons look well-aligned with the interface below them on a typical phone form factor. On a tablet, they extend beyond the text. Should we use the content margin dimensions to keep the Quick Action buttons aligned with the interface below them on large sccreen? See the screenshots below for illustration.

10362_margin_button

The same situation applies to the site title and address. See the screenshots below for illustration.

10362_margin_text

@theck13 theck13 removed their assignment Aug 21, 2019

@peril-wordpress-mobile

This comment has been minimized.

Copy link

commented Aug 22, 2019

You can test the changes on this Pull Request by downloading the APK here.

@khaykov

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

Thanks for a thorough review, @theck13 !

The touch feedback for the Switch Site button isn't what I would expect from a text button. Material guidelines show a rectangular background when pressing text buttons. Should we change the background from ?attr/selectableItemBackgroundBorderless to ?attr/selectableItemBackground and mimic the background border of the Change Photo text button on the Me tab? See the screenshots below for illustration.

Good catch! I changed the background and added a little bit of padding to the sides so it will look better.

The Quick Action buttons look well-aligned with the interface below them on a typical phone form factor. On a tablet, they extend beyond the text. Should we use the content margin dimensions to keep the Quick Action buttons aligned with the interface below them on large sccreen? See the screenshots below for illustration.

I agree, but honestly not sure how to approach this. We are using layout_weight and evenly spreading button, so any margin will mess this up. I'm open to ideas :)

I added content margin to the site title and address tho :)

Screenshot_1566444490

@osullivanchris

This comment has been minimized.

Copy link

commented Aug 22, 2019

Great details to spot, thanks @theck13

Agree with all of those. Looks like the button touch area, and the content padding have already been adjusted.

I agree, but honestly not sure how to approach this. We are using layout_weight and evenly spreading button, so any margin will mess this up. I'm open to ideas :)

Is it not possible to spread the buttons evenly within an area? Not sure how it would work in code. But could it be a separate div with margin outside of it, and the items spaced equally within it? Alternatively I could provide paddings between the buttons, but not sure that will be as successful across device sizes as evenly spacing them

@planarvoid planarvoid removed their request for review Aug 22, 2019

@khaykov

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

@osullivanchris Hm. Maybe I'm missing something?

Currently, buttons are spread evenly, like this - you can see four even boxes, and buttons float in the center of each box.
Image from Gyazo

If we add padding or margin, the container with 4 boxes will get smaller, boxes will get smaller, but the buttons will still float in the center (will be spread evenly withing smaller container).
Image from Gyazo

After adding padding, we can tell first and last button to stick to sides, instead of floating in the center of their box, this way we will get them to be extacly where we want, but this will make other buttons look weird:
Image from Gyazo

We can also make two buttons in the center gravitate towards their side of the screen. Looks more symmetrical, but we have a weird gap.
Image from Gyazo

And there is the case with 3 buttons, that some users will see
Image from Gyazo

@osullivanchris

This comment has been minimized.

Copy link

commented Aug 22, 2019

If we add padding or margin, the container with 4 boxes will get smaller, boxes will get smaller, but the buttons will still float in the center (will be spread evenly withing smaller container).

this sounds like what I would expect

I'm picturing it that as the screen width increases there becomes a larger margin area (in blue) where content will not spread
Screenshot 2019-08-22 at 18 12 09

If as the screen width increases and the buttons are spread equally, the outside button doesn't line up with the left edge of the content below it, thats fine by me. Example here
Screenshot 2019-08-22 at 18 12 17

I don't think the outside of the buttons need to be strictly aligned to the edge of the list content. I would rather they end up narrower than the list, rather than spreading out wider than it. If this is confusing I can explain better on a call tomorrow or something! :)

@khaykov

This comment has been minimized.

Copy link
Member Author

commented Aug 22, 2019

@osullivanchris Got it! For some reason, I thought that buttons should align with rest of the text :)
I added a content margin, and it looks like this on tablet:
Image from Gyazo

And like this on the phone:
Image from Gyazo

@theck13
Copy link
Contributor

left a comment

Thanks for all the changes, @khaykov! I think the code looks good. I only have one more change request remaining.

Update WordPress/src/main/res/layout/my_site_fragment.xml
Co-Authored-By: Tyler Heck <theck13@users.noreply.github.com>
@theck13

This comment has been minimized.

Copy link
Contributor

commented Aug 22, 2019

@osullivanchris, I see you approved these changes a week ago, but I wanted to confirm you approve the changes of the last two days as well. Are we good to merge this?

@osullivanchris

This comment has been minimized.

Copy link

commented Aug 22, 2019

@khaykov ah cool, glad we were able to clear it up. Looks good!

@theck13 yep I'm happy with it, thanks for checking

@theck13 theck13 merged commit 2b1ccf7 into develop Aug 22, 2019

5 checks passed

Peril Found some issues. Don't worry, everything is fixable.
Details
ci/circleci: Installable Build Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: strings-check Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details

@theck13 theck13 deleted the feature/quick-actions-v1 branch Aug 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.