refactor of Frame (redux component) #8225

NejcZdovc · 2017-04-11T12:01:40Z

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).

Resolves #8392

Test Plan:
test that the browser starts normally

NejcZdovc · 2017-04-12T07:23:43Z

blocked by #8238

NejcZdovc · 2017-04-26T09:54:21Z

@bsclifton @bridiver @bbondy unit test is still failing, but we need to review this and merge it, otherwise merge conflicts will start popping up. There is still room for improvements, but would like to do it multiple steps. This PR implemented redux component and simplify as much as possible.

Next PR should be focused on simplifying actions, because right now we are sending all frameProps or all settings instead of only things that we need in the store/reducers. When that PR is in we could continue with Frame simplification.

Unit test problem
Problem is with calling a function on Frame component that is wrapped into ReduxComponent via connect, so you can’t call function directly like this
https://github.com/brave/browser-laptop/pull/8225/files#diff-a6847547aef4f43c811e9e97dcb0322cL46
or spy on it like this
https://github.com/brave/browser-laptop/pull/8225/files#diff-a6847547aef4f43c811e9e97dcb0322cL45

luixxiul · 2017-04-26T11:22:12Z

sorry for stepping in but if you have several PRs expecting I think you are expected to work on another base branch like redux-refactoring and solve conflicts later by yourself (sorry if that is not a common practice on github). See: #8443 and the PRs referred there. I think it is not a good practice to hurry people to review PRs because they have been already so busy working on something. The reason why I claim so is that I'm afraid of some kind of regressions could happen. Especially without test plans there should be much possibility that regressions would happen.

NejcZdovc · 2017-04-26T12:39:07Z

@luixxiul thank you for feedback. I understand that and I added label for a review only today. I don't expect or demand from them to do it today 😃. All three of them already helped me a lot on this PR, for which I am really grateful.

Why I think we can't go with a base branch is that things couldn't be merged after redux is done. There would be to much mess and conflicts. What we are doing with redux components and moving things into state is a new way of writing components. Because of that some things are not compatible and we will change a lot functions and how they are called. And if we don't merge this on a frequent basis, other developers would adding code that will be then removed for example or using functions that are not valid anymore. This way they will waste time writing an old code.

Because of that we wanted to approach this redux refactoring with small PR's and not doing everything in one big swing. @bridiver did already a lot of amazing work for url bar (changing to redux and optimizing things) and it's in master already. This way developers can start using this new approach faster.

For test's I didn't add any specific one (except for my new code for the state), because we already have test's for frames and with this PR I didn't implement any new feature, just move things around and implement redux.

luixxiul · 2017-04-26T13:11:56Z

I didn't implement any new feature, just move things around.

Sometimes even doing so would cause troubles (slow performance etc.). If you improve something with this PR would you mind adding a manual test plan even if it would be a tiny one? like:

1. Clear your profile
2. Open the browser
3. Make sure the browser starts in/under ... seconds

Otherwise more simply: test that the browser starts normally. That would help, thanks.

luixxiul · 2017-04-26T13:12:51Z

app/renderer/components/common/messageBox.js

@@ -3,20 +3,34 @@
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

 const React = require('react')
-const ImmutableComponent = require('../immutableComponent')
+const {StyleSheet, css} = require('aphrodite')


Can't it be aphrodite/no-important?

@luixxiul not sure, we need to ask @bsclifton about this code, I just reordered imports.

@luixxiul maybe? we could do that, but it would require testing. I would vote to leave it as is. IMO, using no-important takes away the benefit we get by using Aphrodite.

We introduced Aphrodite to our code base because we were getting destroyed by regressions and the goal (I thought) was to make styles NOT overridable. When it's no-important, styles might be overridden (which can cause regression).

OK, I understand. I proposed it so we could remove styles in the existing LESS files at the same time. It is fine to replace them with later.

Otherwise: I'm thinking we would set no-important to the general components (such as ones in common folder) and not to other specific ones, considering the possibility of removing !important from everything.

That means:

aphrodite for files in app/renderer/components/ such as releaseNotes.js

aphrodite/no-important for files in app/renderer/components/common/ such as dropdown.js

What do you guys think? @bsclifton @cezaraugusto @NejcZdovc. By doing so we could avoid overwriting !important with !important, which is obviously not a good habit on CSS.

The thing is that I noticed that it seemed to be impossible to cascade styles with cx as we usually do with CSS, which I noticed during refactoring modalOverlay.js and paymentTab.js. eg:

<div className={cx({ [modalOverlay__generalHeaderStyles]: true, [css(styles.modalOverlay__paymentHistoryHeaderStyles)]: true })}

I thought that kind of styling should work, overwriting the styles specified with modalOverlay__generalHeaderStyles, but I could not find the proper way, instead of creating another wrapper div for that div. If any, please let me know. @bsclifton @cezaraugusto @NejcZdovc

We could discuss the thing on another issue if you wish.

I think I need to understand more about Aphrodite and the use cases for this (I think I'm missing something). Let's definitely chat more about it 😄

yep per @luixxiul example, Aphrodite will concatenate styles and the last would take precedence. I think that doesn't happen with cx but definitely happen with standard css(style1, style2).

I'm against !important whenever possible. I would compare it to JS eval(), it takes precedence whenever it's defined, and that's a bad pattern. It's a feature that might be useful for some edge-cases until we aren't using Aphrodite 100% but long term we should avoid.

For new components, !important is a no-op but this is a huge PR and if that was previously defined we should keep it as-is. As we head closer to 1.0 we should avoid re-work and reduce regression risk at all costs. However, wouldn't mind reviewing a follow-up removing that, but for this PR I think we are fine with what we have.

I think that doesn't happen with cx

Can we improve it to support cascading styles?

cx() is just a lib to apply styles conditionally, so it's affected by the same rules as CSS like deeply nested selectors taking precedence, and !important which overrides everything.

I don't think there's anything we can do with that but just avoid adding new LESS stuff and keep using until it can be removed.

@cezaraugusto I understand how !important can be viewed negatively like eval(). However, until we're fully switched over, I can see having !important be a great way to prevent regressions. When we get close to 100% Aphrodite, then you just switch aphrodite => aphrodite/no-important 😄

All that said, we've done an amazing job with our styles since introducing Aphrodite. Before (2016), we were always fighting regressions. Now, we rarely have them (if ever), even with no-important. I fully support whatever makes the most sense 😄

NejcZdovc · 2017-04-26T13:14:59Z

@luixxiul sure I can do this, not a problem. And I agree completely with you.

luixxiul · 2017-04-26T15:54:24Z

@NejcZdovc please make sure to add test steps to the 1st post for fixes of these fails on travis: https://travis-ci.org/brave/browser-laptop/jobs/226054730#L2461

NejcZdovc · 2017-05-01T08:15:43Z

After discussion with @bsclifton unit test was disabled for frame for now. We will address issue of fixing unit tests for redux components in a different PR. We will track all this problems in #8528

bsclifton · 2017-05-02T23:48:19Z

app/common/state/siteState.js

@@ -0,0 +1,18 @@
+const assert = require('assert')
+const { makeImmutable, isMap, isList } = require('./immutableUtil')


nitpick- I don't think it's documented anywhere, but we don't have spaces when doing destructuring in most of our code. So it could look like this instead:
const {makeImmutable, isMap, isList} = require('./immutableUtil')

@bridiver

Resovles brave#8392 Auditors: @bridiver @bbondy Test plan: - test should be green

bsclifton

Huge PR! Great work on this 😄

We still need to revisit the tests for frame. I tried a few things and didn't have much luck. I believe you created an issue to track the need to fix the tests (but if not let's make sure there is one)

bsclifton · 2017-05-02T23:52:26Z

js/components/main.js

-      numberOfMatches: -1,
-      activeMatchOrdinal: 0
-    }))
+    frameStateUtil.onFindBarHide(activeFrame.get('key'))


nice cleanup 👍

bsclifton · 2017-05-03T06:42:30Z

app/renderer/components/common/messageBox.js

@@ -3,20 +3,34 @@
 * You can obtain one at http://mozilla.org/MPL/2.0/. */

 const React = require('react')
-const ImmutableComponent = require('../immutableComponent')
+const {StyleSheet, css} = require('aphrodite')


@cezaraugusto I understand how !important can be viewed negatively like eval(). However, until we're fully switched over, I can see having !important be a great way to prevent regressions. When we get close to 100% Aphrodite, then you just switch aphrodite => aphrodite/no-important 😄

All that said, we've done an amazing job with our styles since introducing Aphrodite. Before (2016), we were always fighting regressions. Now, we rarely have them (if ever), even with no-important. I fully support whatever makes the most sense 😄

bsclifton · 2017-05-03T06:47:04Z

app/renderer/lib/domUtil.js

@@ -0,0 +1,7 @@
+module.exports.createWebView = () => {


this is awesome- great use of a util. People might think it's silly having this, but it makes mocking VERY easy 😄 👍

bsclifton · 2017-05-03T08:06:57Z

js/components/main.js

-                enableNoScript={siteSettingsState.isNoScriptEnabled(this.props.appState, this.frameSiteSettings(frame.get('location')))}
-                braveryDefaults={braveryDefaults}
-                isPreview={frame.get('key') === this.props.windowState.get('previewFrameKey')}
-                isActive={frameStateUtil.isFrameKeyActive(this.props.windowState, frame.get('key'))}
              />)


Bye bye code! 🗑
😄

NejcZdovc · 2017-05-03T10:59:19Z

I will merge this. If we find some more improvements, we can do follow up later on. I need this PR merged so that I can continue with work on #8651. @bsclifton I added note about unit test in #8528.

bsclifton · 2017-05-03T16:18:38Z

Perfect, thanks @NejcZdovc 😄

NejcZdovc added the PR/work-in-progress ⚒ label Apr 11, 2017

NejcZdovc self-assigned this Apr 11, 2017

NejcZdovc force-pushed the feature/redux-frame branch from 3e38b43 to 3b0dd8d Compare April 11, 2017 13:29

NejcZdovc added the PR/blocked label Apr 12, 2017

NejcZdovc changed the base branch from master to about-pages April 12, 2017 14:42

NejcZdovc force-pushed the feature/redux-frame branch 2 times, most recently from 820518d to 8d07cee Compare April 12, 2017 15:27

bridiver force-pushed the about-pages branch 2 times, most recently from 74b26a1 to 7168987 Compare April 12, 2017 18:07

NejcZdovc changed the base branch from about-pages to master April 13, 2017 10:28

NejcZdovc force-pushed the feature/redux-frame branch from 8d07cee to fb31a99 Compare April 13, 2017 10:43

NejcZdovc removed the PR/blocked label Apr 13, 2017

NejcZdovc force-pushed the feature/redux-frame branch 11 times, most recently from f29bc26 to a5aeadf Compare April 19, 2017 06:11

NejcZdovc added this to the 0.15.1 milestone Apr 19, 2017

NejcZdovc force-pushed the feature/redux-frame branch from a5aeadf to c6206a9 Compare April 19, 2017 18:21

bbondy force-pushed the master branch from f0df59d to 70e5d77 Compare April 23, 2017 04:21

NejcZdovc force-pushed the feature/redux-frame branch 3 times, most recently from e509fd2 to cd44019 Compare April 23, 2017 21:39

NejcZdovc requested a review from bbondy April 26, 2017 09:50

NejcZdovc added PR/needs-QA-attention ☕ and removed PR/work-in-progress ⚒ labels Apr 26, 2017

NejcZdovc force-pushed the feature/redux-frame branch from dc3207d to e18f46d Compare April 26, 2017 09:51

NejcZdovc force-pushed the feature/redux-frame branch from e18f46d to bde4204 Compare April 26, 2017 13:00

luixxiul reviewed Apr 26, 2017

View reviewed changes

NejcZdovc force-pushed the feature/redux-frame branch from bde4204 to 1b1998f Compare April 26, 2017 15:27

NejcZdovc mentioned this pull request Apr 27, 2017

Redux refactoring tracking issue #8528

Closed

51 tasks

luixxiul added the PR/needs-rebase ۞ label Apr 29, 2017

NejcZdovc force-pushed the feature/redux-frame branch from 1b1998f to 54f34ef Compare April 30, 2017 08:27

NejcZdovc removed the PR/needs-rebase ۞ label Apr 30, 2017

NejcZdovc force-pushed the feature/redux-frame branch 2 times, most recently from dd129e1 to 16dd2a3 Compare May 1, 2017 08:14

cezaraugusto added the refactoring/redux-component label May 1, 2017

bsclifton reviewed May 2, 2017

View reviewed changes

Frame -> redux component

913813f

Resovles brave#8392 Auditors: @bridiver @bbondy Test plan: - test should be green

NejcZdovc force-pushed the feature/redux-frame branch from 16dd2a3 to 913813f Compare May 3, 2017 05:08

bsclifton approved these changes May 3, 2017

View reviewed changes

NejcZdovc merged commit 7f7d871 into brave:master May 3, 2017

luixxiul removed the PR/needs-QA-attention ☕ label May 3, 2017

NejcZdovc mentioned this pull request May 5, 2017

Redux component - Frame #8392

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor of Frame (redux component) #8225

refactor of Frame (redux component) #8225

NejcZdovc commented Apr 11, 2017 •

edited

Loading

NejcZdovc commented Apr 12, 2017

NejcZdovc commented Apr 26, 2017 •

edited

Loading

luixxiul commented Apr 26, 2017

NejcZdovc commented Apr 26, 2017 •

edited

Loading

luixxiul commented Apr 26, 2017

luixxiul Apr 26, 2017

NejcZdovc Apr 26, 2017

bsclifton Apr 27, 2017

luixxiul Apr 27, 2017 •

edited

Loading

bsclifton Apr 27, 2017

cezaraugusto Apr 27, 2017

luixxiul Apr 28, 2017

cezaraugusto Apr 28, 2017

bsclifton May 3, 2017

NejcZdovc commented Apr 26, 2017 •

edited

Loading

luixxiul commented Apr 26, 2017

NejcZdovc commented May 1, 2017

bsclifton May 2, 2017

NejcZdovc May 3, 2017

bsclifton left a comment

bsclifton May 2, 2017

bsclifton May 3, 2017

bsclifton May 3, 2017

bsclifton May 3, 2017

NejcZdovc commented May 3, 2017

bsclifton commented May 3, 2017

		@@ -0,0 +1,18 @@
		const assert = require('assert')
		const { makeImmutable, isMap, isList } = require('./immutableUtil')

refactor of Frame (redux component) #8225

refactor of Frame (redux component) #8225

Conversation

NejcZdovc commented Apr 11, 2017 • edited Loading

NejcZdovc commented Apr 12, 2017

NejcZdovc commented Apr 26, 2017 • edited Loading

luixxiul commented Apr 26, 2017

NejcZdovc commented Apr 26, 2017 • edited Loading

luixxiul commented Apr 26, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luixxiul Apr 27, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Apr 26, 2017 • edited Loading

luixxiul commented Apr 26, 2017

NejcZdovc commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsclifton left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented May 3, 2017

bsclifton commented May 3, 2017

NejcZdovc commented Apr 11, 2017 •

edited

Loading

NejcZdovc commented Apr 26, 2017 •

edited

Loading

NejcZdovc commented Apr 26, 2017 •

edited

Loading

luixxiul Apr 27, 2017 •

edited

Loading

NejcZdovc commented Apr 26, 2017 •

edited

Loading