-
Notifications
You must be signed in to change notification settings - Fork 295
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
Page Speed widget - "Mobile" & "Desktop" icon change #3162
Comments
@Pratheep-lab are the current icons not from Material? I believe whatever is there was provided by @aarondruck in the original designs but perhaps he can confirm. |
Sorry, I think we can close this. I was mostly tripped up on the
difference between the mobile icons, of what was designed and what's in
production. But it's minimal, so we can just close this.
Original icon:
[image: image.png]
Icon in production:
[image: image.png]
…On Tue, Apr 20, 2021 at 7:24 AM Evan Mattson ***@***.***> wrote:
Assigned #3162 <#3162> to
@aarondruck <https://github.com/aarondruck>.
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#3162 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAURNSU2QIQW6MKYNTOLWRLTJWFDHANCNFSM427VKPLQ>
.
|
@aarondruck here they are side-by-side which makes it a bit easier to compare. The color looks more balanced with the new phone icon, but I think I prefer the laptop icon to the monitor. How about What do you think? |
The *Smartphone* and *Computer* icons work, thank you for looking into it!
…On Mon, May 3, 2021 at 5:16 AM Evan Mattson ***@***.***> wrote:
@aarondruck <https://github.com/aarondruck> here they are side-by-side
which makes it a bit easier to compare.
[image: image]
<https://user-images.githubusercontent.com/1621608/116874155-80d0d500-ac21-11eb-962d-3b3ffb570021.png>
The color looks more balanced with the new phone icon, but I think I
prefer the laptop icon to the monitor.
How about smartphone and computer icons?
[image: image]
<https://user-images.githubusercontent.com/1621608/116874699-69deb280-ac22-11eb-8a0b-a7681ec2e418.png>
[image: image]
<https://user-images.githubusercontent.com/1621608/116874709-6cd9a300-ac22-11eb-9919-6155fe7568b5.png>
What do you think?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#3162 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAURNSQGHG3S3SGLJ2SPT4DTL2HY7ANCNFSM427VKPLQ>
.
|
IB ✔️ |
This is ready to go into CR but as it is outside of the current sprint I'm leaving it in execution |
Setting to Code Review as I'm unassigning tickets from myself 😄 |
Adding to the 1.40.0 release since this is a trivial enhancement and good to go. cc @aaemnnosttv |
QA Update: ❌@aaemnnosttv @felixarntz in my opinion the icons look a lot smaller than the previous ones. Maybe it's my eyes 😃 What are your thoughts? They appear small on desktop and mobile. |
Thanks @wpdarren, they are a bit smaller now you're right. I've opened a PR to fix this 👍 |
On the Page Speed widget found at the bottom of the SK dashboard, the mobile and desktop icons need to be changed to standard material design icons.
Current screenshot showing Page speed widget:
Do not alter or remove anything below. The following sections will be managed by moderators only.
Acceptance criteria
computer
smartphone
currentColor
instead of a fixed hex codeImplementation Brief
assets/svg/device-size-mobile-icon.svg
by thesmartphone
icon in the AC.assets/svg/device-size-desktop-icon.svg
by thecomputer
icon in the AC.fill
attribute with thecurrentColor
value to the lastpath
element.assets/sass/components/global/_googlesitekit-DeviceSizeTabBar.scss
,color
styles tosvg
, both including the one for.mdc-tab--active
, having the same value as the correspondingsvg path fill
styles.fill
styles forsvg path
, and the resulting emptypath
selector.Test Coverage
Visual Regression Changes
QA Brief
On the Page Speed widget found at the bottom of the SK dashboard:
Changelog entry
The text was updated successfully, but these errors were encountered: