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

NTP: Add "Customize" text to dashboard settings icon #5103

Merged
merged 3 commits into from
Apr 2, 2020
Merged

Conversation

cezaraugusto
Copy link
Contributor

Fix brave/brave-browser#4930

Submitter Checklist:

Test Plan:

To see changes live without building Brave, run

npm run storybook

Changes in this PR should match screenshot

Screen Shot 2020-03-30 at 16 16 14

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@bsclifton
Copy link
Member

oooo this is nice ❤️

Copy link

@karenkliu karenkliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cezaraugusto ! This update looks great! The spacing and everything seems right; the only thing you're missing is a divider between the 'Customize' button and the rest of the browser buttons:
vertical divider
border: 1px solid rgba(255, 255, 255, 0.6); height: 24px; width: 1px;

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This'll be a great change, nice work 😄 Besides what @karenkliu shared, I think it might be good to put user-select: none on the actual div around Customize so that it's not click drag selectable

Also the focus ring only goes on the icon itself. @karenkliu should this go around the whole thing? or only be on the icon?
Screen Shot 2020-03-31 at 12 57 55 AM

@karenkliu
Copy link

This'll be a great change, nice work 😄 Besides what @karenkliu shared, I think it might be good to put user-select: none on the actual div around Customize so that it's not click drag selectable

Also the focus ring only goes on the icon itself. @karenkliu should this go around the whole thing? or only be on the icon?
Screen Shot 2020-03-31 at 12 57 55 AM

@bsclifton @cezaraugusto Around the whole thing! The entire clickable button area.
Screen Shot 2020-03-31 at 10 00 37 AM

@cezaraugusto cezaraugusto force-pushed the ca-4930 branch 2 times, most recently from 39b3052 to 9c02749 Compare April 1, 2020 21:44
@cezaraugusto
Copy link
Contributor Author

Thanks team, updated per screenshot. If unsure, npm run storybook should display the same output.

Screen Shot 2020-04-01 at 18 44 13

Copy link

@karenkliu karenkliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @cezaraugusto Sorry, one more thing - doesn't look like the right font. Can you check and make sure it's font-family: Poppins-SemiBold; font-size: 13px; font-weight: 600;?
Also both the icon and text should be #FFFFFF 80% opacity, and then 100% opacity on hover

@cezaraugusto
Copy link
Contributor Author

Thanks @karenkliu for the quick review. Feedback applied.

Current result:

Screen Shot 2020-04-01 at 19 23 40

Also looking good in RTL:

‏لقطة الشاشة ٢٠٢٠-٠٤-٠١ في ١٩ ٢٠ ٣١

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, pending CI and review from @karenkliu 😄

Nice work!

@karenkliu
Copy link

looks good to me!

@cezaraugusto
Copy link
Contributor Author

Thanks team!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add "Customize" text to NTP dashboard settings icon to make it more discoverable
3 participants