Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upMove top navigation outside the main.js #8149
Conversation
|
Please create an issue for tracking/QA, thanks. |
|
Will do, but first we need to define some more things |
NejcZdovc
force-pushed the
NejcZdovc:feature/redux-navigator
branch
2 times, most recently
from
7696c59
to
db96ab0
NejcZdovc
changed the title
NejcZdovc
force-pushed the
NejcZdovc:feature/redux-navigator
branch
from
db96ab0
to
5aea176
|
Files move was moved into a separate PR #8187, will rebase to the master after that PR is in. This PR will now focus only on moving logic into new component. For redux implementation I will create separate PR. |
| testId='braveShieldButton' | ||
| className={cx({ | ||
| navbutton: true, | ||
| braveShieldsDisabled: shieldDisabled, |
This comment has been minimized.
This comment has been minimized.
bridiver
Apr 10, 2017
Collaborator
I think braveShieldsDisabled and braveShieldsDown can be too easily confused and the button doesn't need to know or care why shields are disabled. I would suggest combining into a single setting and method that handles both cases
This comment has been minimized.
This comment has been minimized.
NejcZdovc
Apr 10, 2017
Author
Member
we are using different css for this two states. Icon is different color if shield is down or disabled
This comment has been minimized.
This comment has been minimized.
bridiver
Apr 10, 2017
Collaborator
ok, the naming is a little confusing, but I guess it matches what is there now
| * License, v. 2.0. If a copy of the MPL was not distributed with this file, | ||
| * You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| const frameStateUtil = require('../../../js/state/frameStateUtil') |
This comment has been minimized.
This comment has been minimized.
bridiver
Apr 10, 2017
Collaborator
I think we would like to avoid adding new *Util classes. Maybe add this to frameState.js and pass in state and frameKey?
This comment has been minimized.
This comment has been minimized.
NejcZdovc
Apr 10, 2017
Author
Member
Yeah I wanted to create util for now and then when moving to redux create shield state. What do you think should we move this into frame or shield state? Keep in mind that we have a lot of logic for the panel
This comment has been minimized.
This comment has been minimized.
bridiver
Apr 10, 2017
Collaborator
shieldState is fine, but I'd still like to pass in the frame for now vs activeRequestedLocation. Can we also change the name to braveShieldsEnabled? !braveShieldsDisabled is harder to read
NejcZdovc
force-pushed the
NejcZdovc:feature/redux-navigator
branch
from
5aea176
to
633a2aa
|
@bridiver will create a new PR, so that branch will have the correct name, also travis is not triggering. |
Moves navigation logic from main.js to new component
NejcZdovc commentedApr 9, 2017
•
edited
git rebase -ito squash commits (if needed).Resolves #8189