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

Get rid of magic numbers associated with downloads bar #3604

Closed
bsclifton opened this issue Aug 31, 2016 · 6 comments
Closed

Get rid of magic numbers associated with downloads bar #3604

bsclifton opened this issue Aug 31, 2016 · 6 comments

Comments

@bsclifton
Copy link
Member

bsclifton commented Aug 31, 2016

The code in Brave is using 50 pixels as the height in several places:

This code needs to be updated to fetch the value via CSS variable. Here is a great example of that:
https://github.com/brave/browser-laptop/blob/master/js/components/bookmarksToolbar.js#L287

@maazadeeb
Copy link
Contributor

Hi @bsclifton,

I would like to take this up. I have gone through the contribution guide. Could you guide me about the further course the action?

@bsclifton
Copy link
Member Author

bsclifton commented Oct 3, 2016

Hi @maaz93

I'd love to help get you started with this change 😄

Have you got the project working from source? That would be the first step (what OS are you using?).

Next up, you'll want to create a variable in less/variables.less to hold this value. You should be able to then reference the value similar to the example linked above in js/components/bookmarksToolbar.js

Let me know if you have any specific questions or if any of the above is unclear 😄

@maazadeeb
Copy link
Contributor

maazadeeb commented Oct 3, 2016

Hi @bsclifton

These instructions are sufficient for me to get started. I'm using a Mac. I'll get the project running from source and start working. I'll contact you on this thread, if need be! Thanks 😄

@bsclifton
Copy link
Member Author

@maaz93 how's it going? Hope all is well- let me know if you have any questions 😄

@maazadeeb
Copy link
Contributor

@bsclifton I was able to get the project running from source. I'm new to the react ecosystem, so I was just looking around in the code base. But, I realized that this bug doesn't seem to require any react knowledge. Right?

I went through files to find any hardcoded 50 pixels usage. I believe this exists only in the 2 files mentioned in the bug description. In order to test my change, I tried to hit the breakpoints in these functions. I was able to hit this one

https://github.com/darkdh/browser-laptop/blob/9e02263ed5c5c0646429ccf885560f319c14f17e/js/contextMenus.js#L1095

I was unable hit the other two

const downloadsBarHeight = 50

https://github.com/darkdh/browser-laptop/blob/9e02263ed5c5c0646429ccf885560f319c14f17e/js/contextMenus.js#L1083

Could you guide me on this ?

@bsclifton
Copy link
Member Author

bsclifton commented Oct 10, 2016

Hi @maaz93

You can breakpoint debug any of the code running in the render process from inside the browser. In order to breakpoint debug the browser process, you can try Visual Studio Code. We have some documentation that I hope is useful here:
https://github.com/brave/browser-laptop/blob/master/docs/debugging.md

Another good way to debug that I use is to just drop some console.log statements in the code. You'll see the output in two places:

  • system console (where you ran npm start) for the browser process
  • the electron devtools (Shift + F8, go to console) for the renderer process

If you're working on an about: page (like about:history), there's a third place to check. You'll see output in the content devtools (F12 or cmd+opt+i)

Hope this helps 😄

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

No branches or pull requests

3 participants