-
Notifications
You must be signed in to change notification settings - Fork 975
Fix margin around the button on about:safebrowsing & Add browserButton_subtleItem fixing regressions #9026
Fix margin around the button on about:safebrowsing & Add browserButton_subtleItem fixing regressions #9026
Conversation
js/about/safebrowsing.js
Outdated
</div> | ||
? <section> | ||
<div className='buttons'> | ||
<Button className='subtleButton' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could we take advantage of this PR and add subtleButton to <BrowserButton>
component?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah right, I've totally forgot about that component :-p
I'll replace it with that, adding subtleButton. Thanks for heads up.
on the 2nd commit I fixed several regressions introduced with #8568 for 0.15.3, which seem to me a not blocker. I think it's fine to wait for 0.16 series. |
paddingLeft: '16px', | ||
|
||
// cf: https://github.com/brave/browser-laptop/blob/548e11b1c889332fadb379237555625ad2a3c845/less/button.less#L98 | ||
minWidth: `calc(${globalStyles.spacing.defaultFontSize} * 6)`, // issue #6384 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice catch
|
||
browserButton_subtleItem: { | ||
// cf: https://github.com/brave/browser-laptop/blob/548e11b1c889332fadb379237555625ad2a3c845/less/button.less#L151-L152 | ||
background: '#ccc', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should belong to global styles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed with 03a9abd
Closes #9072 Addresses #9071 Also: - Add lines in the same way as LESS files - Add comments for refenrence (We could remove them after refactoring work) - Fix regressions on browserButton_default - Add globalStyles.button.color to replace it with globalStyles.button.default.color - Fix a typo Auditors: Test Plan 1: (0. Take a screenshot of the buttons section on about:styles before testing the PR for later use) 1. Open about:styles 2. Click "buttons" 3. Take another screenshot of the section 4. Compare the two screenshots and make sure they look same Test Plan 2: 1. Open about:styles 2. Toggle developer tools 3. Make sure button's padding is set to '5px 16px' Test Plan 3: 1. Open downloadme.org 2. Click "Advanced" button 3. Make sure the button is not pushed down 4. Click "Hide advanced" button 5. Make sure the button does not move
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++
@cezaraugusto thanks for review. |
On the 1st commit:
Fix margin around the button on about:safebrowsing
On the 2nd commit:
Add browserButton_subtleItem fixing regressions
Closes #8945
Closes #9072
Addresses #9025
Addresses #9071
Auditors:
Test Plan 1:
Test Plan 2:
Test Plan 3:
Submitter Checklist:
git rebase -i
to squash commits (if needed).Reviewer Checklist:
Tests