Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Fixing magic number bug in downloads bar #4837

Merged
merged 1 commit into from
Oct 17, 2016
Merged

Conversation

maazadeeb
Copy link
Contributor

@maazadeeb maazadeeb commented Oct 16, 2016

  • 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).
  • Ran git rebase -i to squash commits (if needed).

Test Plan:

Fixes #3604

Added a new variable for downloads bar height in variables.less and consumed this across the codebase where 50px was hardcoded for this.

Auditors: @bsclifton

@bsclifton bsclifton self-assigned this Oct 16, 2016
@bsclifton bsclifton added this to the 0.12.6dev milestone Oct 16, 2016
@bsclifton
Copy link
Member

bsclifton commented Oct 17, 2016

I pulled the code locally and manually tested each of the scenarios (as-is with 50px and then setting to another height):

  1. The actual download bar itself
  2. Positioning of the autofill context menu when filling a form
  3. Positioning of the username context menu (when using built-in pwd mgr)
  4. Title that shows in bottom left when doing a mouse over for an anchor tag

We're wrapping up our 0.12.5 release, this can go into our next release 0.12.6

PR is ready for merge. Thanks, @maaz93! 😄

@maazadeeb
Copy link
Contributor Author

@bsclifton Thanks! This would be my first open source contribution! I will continue fixing more bugs to understand the codebase more 😄

@bbondy
Copy link
Member

bbondy commented Oct 17, 2016

great cleanup @maaz93, thanks a ton!
When the downloads bar was first added, chromium didn't have support for css variables like this.

@bbondy bbondy merged commit 75b91b4 into brave:master Oct 17, 2016
@maazadeeb
Copy link
Contributor Author

You're welcome @bbondy ! Really happy you liked the clean up! 😄

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

Successfully merging this pull request may close these issues.

4 participants