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

Basic tab structure #1

Merged
merged 4 commits into from
Dec 1, 2015
Merged

Basic tab structure #1

merged 4 commits into from
Dec 1, 2015

Conversation

KevinGrandon
Copy link
Contributor

No description provided.

@bbondy
Copy link
Member

bbondy commented Nov 29, 2015

Obviously still a lot of unhooked parts, but definitely a step towards the right direction.

One thing I'd like to see changed before landing is to move the state outside of tabs.js.
State would change in ui.tabs.

Ideally we'll have all components with no state inside of them and fully loaded from prop changes.

@KevinGrandon
Copy link
Contributor Author

State for tab dragging and dropping? We can move that out.

It seems like it might be a lot of work to have state in ui.tabs, wouldn't we need to synchronize that with the frames as well?

How do we feel about tacking on additional data to each frame? E.g., frames.[n].tabIsDragging|tabIsDraggingOverLeftHalf|tabIsDraggingOverRightHalf

@bbondy
Copy link
Member

bbondy commented Nov 30, 2015

As silly as it sounds for this case, yes I was hoping that we could have fully referentially transparent components. Please spend a few minutes to read this to see if it might change your mind. I could see doing an exception here leading to doing exceptions elsewhere. The hope would be to change partially or fully to React stateless components which they stated in 0.14 release notes that there would be further performance optimizations related to them.

Note that ImmutableComponent doesn't even consider state for detecting changes right now.
https://github.com/brave/browser-electron/blob/master/js/components/immutableComponent.js

I don't think it would be that hard to do the state changes in the store, it would just require a single action which can manage each of those 3 states.

I won't try to stop you from landing, but I'd like to give stateless components a shot.

@KevinGrandon
Copy link
Contributor Author

I agree that we should move the state out, just trying to figure out the right place for it :)

Should it go into ui.tabs or be tacked onto each frame? If we put it on ui.tabs, then we'd need to synchronize the tabs for each frame operation that we do currently right? (Adding/removing/crashing/etc)

@bbondy
Copy link
Member

bbondy commented Nov 30, 2015

If we put it on ui.tabs, then we'd need to synchronize the tabs for each frame operation that we do currently right? (Adding/removing/crashing/etc)

hrm not sure I see why, can you explain? Also let me know what you mean by synchronize, might be better for a slack cha if you want higher bandwidth discussion.

})
},

tabDragDraggingOverLeftHalf: function (frameProps) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if left + right could just be combined into a single action with different params?

@bbondy
Copy link
Member

bbondy commented Dec 1, 2015

OK going to merge this in, feel free to do the follow ups if you agree in an audit.

bbondy added a commit that referenced this pull request Dec 1, 2015
@bbondy bbondy merged commit 8ed40b0 into master Dec 1, 2015
@KevinGrandon KevinGrandon deleted the tabs branch December 1, 2015 00:33
This was referenced Jan 24, 2016
@bridiver bridiver mentioned this pull request Feb 11, 2016
@cfumante cfumante mentioned this pull request Apr 1, 2016
@bradleyrichter bradleyrichter mentioned this pull request Jul 4, 2016
bbondy pushed a commit that referenced this pull request Jul 6, 2016
@ianimobli ianimobli mentioned this pull request Aug 6, 2016
@bsclifton bsclifton mentioned this pull request Sep 14, 2016
30 tasks
@kjozwiak kjozwiak mentioned this pull request Sep 7, 2017
16 tasks
@A9G-Data-Droid A9G-Data-Droid mentioned this pull request Mar 30, 2018
stripedpajamas pushed a commit to stripedpajamas/browser-laptop that referenced this pull request Jun 13, 2018
bsclifton pushed a commit that referenced this pull request Aug 6, 2018
add metamask icon for disabled content page
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants