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

Brave Actions sized at 18px and highlighted correctly #1249

Merged
merged 3 commits into from
Jan 17, 2019

Conversation

petemill
Copy link
Member

@petemill petemill commented Jan 8, 2019

Updates brave-extension dep - here's what's in that update: brave/brave-extension@038a43a...70550a1

Fix brave/brave-browser#2295
Fix brave/brave-browser#2807

  • Increases size of icons
  • Custom drawing for buttons' highlight background so that right side is inset, including a right margin within the button itself. The badge can therefore be drawn within this area if it gets too wide.
  • Anchors the badge 12px from right side of highlight / 14px from right side of entire control.
  • Re-centering of icon within custom highlight inset and not within the button rectangle that contains the right inset.

image

image

image

image

image

image

image

image

image

image

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Verified that these changes build without errors on
    • Windows
    • macOS
    • Linux
  • Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests) on
    • Windows
    • macOS
    • Linux
  • Ran git rebase master (if needed).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.
  • Request a security/privacy review as needed.
  • Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone

Test Plan:

See brave/brave-browser#2295

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

@petemill petemill self-assigned this Jan 8, 2019
@srirambv
Copy link
Contributor

srirambv commented Jan 8, 2019

@petemill @rossmoody could the badge not overlap on shields icon but instead make at the edge? The badge on BAT logo doesn't overlap(at least with single digit) instead is just at the border radius. Could the shields also a similar implementation?

@petemill petemill force-pushed the brave-actions-size branch from 1e38265 to 7a14e75 Compare January 8, 2019 14:52
cezaraugusto
cezaraugusto previously approved these changes Jan 8, 2019
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

did a build with brave/brave-extension#89 checked out and confirmed it's working. lgtm

@rossmoody
Copy link
Contributor

@srirambv an overlap is pretty standard for notification elements on icons. the shape of the rewards icon just makes it very difficult without appearing completely off balance

@rossmoody
Copy link
Contributor

@petemill looks good to me. one note, and i'm unsure if it should be tackled in this pr, but we are removing the disabled icon variant (the light grey one). the icons for Rewards and Shields can only ever appear On or Off and the dropdown panel will provide more context to the user.

@petemill
Copy link
Member Author

petemill commented Jan 8, 2019

@srirambv the rewards badge does overlap, but the icon is a triangle so it doesn't look like it does. Please see the mockup at brave/brave-browser#2295 for the coordinates we're aiming for

@petemill petemill force-pushed the brave-actions-size branch 2 times, most recently from 7d5cd18 to d327d93 Compare January 15, 2019 22:18
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.

Changes look great! 😄 Test in both light and dark mode. Compared several sites with what's on Beta at the moment. Even though it's only a little larger it looks amazing. Really nice job on this one 😄 (code changes look good too!)

@bsclifton
Copy link
Member

Manually ran tests (after noticing check above failed). Everything looks good 👍

@bsclifton bsclifton merged commit aa1ab59 into master Jan 17, 2019
@bsclifton bsclifton deleted the brave-actions-size branch January 17, 2019 07:43
bsclifton added a commit that referenced this pull request Jan 17, 2019
Brave Actions sized at 18px and highlighted correctly
bsclifton added a commit that referenced this pull request Jan 17, 2019
Brave Actions sized at 18px and highlighted correctly
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Brave Action notification badge no longer paintable outside button Brave Action Item sizing
6 participants