Finished Aphrodite restyle of safebrowsing.js. Fixes issue #9025 #13592
Conversation
|
Cleaning up open PRs |
js/about/safebrowsing.js
Outdated
| } | ||
|
|
||
| onAdvancedToggle () { | ||
| onAdvancedToggle = () => { |
There was a problem hiding this comment.
To prevent having to call .bind from each usage (at the time, anticipated future use).
| (baseSelector.includes(GLOBALS) ? generateSubtreeStyles(selector) : null) | ||
| } | ||
|
|
||
| export const childSelector = (selector) => { |
There was a problem hiding this comment.
I think this is a leftover. Is this being used elsewhere?
There was a problem hiding this comment.
No. that's a leftover with no cleanup. Removing childSelector
| fontWeight: 400, | ||
| margin: 0, | ||
| padding: 0 | ||
| }, |
There was a problem hiding this comment.
nit: we usually add an extra line break between properties when dealing with styles
There was a problem hiding this comment.
Yes, I saw that on the styling guidelines after the fact. Done.
| [GLOBALS]: { | ||
| '*': { | ||
| color: globalStyles.color.commonTextColor, | ||
| fontFamily: 'Helvetica Neue, Arial, sans-serif', |
There was a problem hiding this comment.
we can replace it with -apple-system, BlinkMacSystemFont, "Segoe UI", "Helvetica Neue", Roboto, Oxygen, Ubuntu, Cantarell, "Fira Sans", "Droid Sans", sans-serif; which is the default font set for our app
| } | ||
| } | ||
| }) | ||
| export default extended.css(styles[GLOBALS]) |
There was a problem hiding this comment.
for parity with other exports we use I think it's preferable to use Node.js module.export utility instead of ES6's so module.exports = extended.css(styles[GLOBALS]) would be better
js/about/safebrowsing.js
Outdated
|
|
||
| class SafebrowsingPage extends React.Component { | ||
| constructor (props) { | ||
| super(props) | ||
| this.state = { | ||
| advanced: false | ||
| } | ||
| let headDiv = document.querySelector('#appContainer') |
There was a problem hiding this comment.
consulting the DOM in React has its cost and we could avoid this by adding a new <div> with the same CSS properties and result is the same i.e.
render () {
return <div className={css(styles.appContainer)}>
<div className={css(styles.error__content)} data-test-id='errorContent'>
...stuff
</div>
</div>| }, | ||
| 'html, body, #appContainer, #appContainer > div': { | ||
| height: '100%' | ||
| }, |
There was a problem hiding this comment.
same as above comment regarding line-break
There was a problem hiding this comment.
This one I might need your help understanding.
|
@cezaraugusto Thank you. Updated with corrections. |
…efactor Finished Aphrodite restyle of safebrowsing.js. Fixes issue #9025
|
@jasonrsadler @cezaraugusto after talking with @NejcZdovc, I uplifted this to 0.23.x to avoid getting ourselves into a nasty merge conflict type situation (like we had in the past when cherry picking commits to older releases) |
cezaraugusto
added
polish
|
Labeled as |
Submitter Checklist:
git rebase -ito squash commits (if needed).Test Plan:
Reviewer Checklist:
Tests
Finishes restyle to Aphrodite. Eliminates dependency to 'less' files.
Requesting Review @luixxiul @bsclifton
Requesting Security Review @diracdeltas
fixes #9025