Update: Keep tabs same size when closing (fixes #6088)

[gyandeeps]

#6088

Same behavior as chrome

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

Test Plan:

1. open 7-8 tabs (donest matter what the content are)
2. move your cursor to the close button of the second tab from left
3. Now as u keep clicking it will maintain the size of the tabs
4. The size will reset as soon as you move your cursor outside the tab area.

After the change:

[cezaraugusto]

We should avoid using forceUpdate() to force re-render, it's ill advised by React itself.

Ref: https://facebook.github.io/react/docs/react-component.html#forceupdate

Normally you should try to avoid all uses of forceUpdate() and only read from this.props and this.state in render().

Could you do it in a way to avoid that? thanks!

[gyandeeps]

I have to use forceUpdate here because we inherit from ImmutableComponent and not ReactComponent. ImmutableComponent checks for props only to make the render update work.
forcedUpdate has been used in other places also (possibly because of the above reason).

[bsclifton]

@gyandeeps the ideal approach would be flux-like and to do the following:

• store the state in our AppStore / AppState. You can view the current layout of the appState here. We'd want to find a home for the new fixTabWidth value
• Inside onFrameClose, create and call a new appAction which has should also have an event based name (as opposed to something like setFixTabWidth). onFrameClose seems OK (In the future, other code may want to process on this event)
• The action then (like other actions) makes a call to dispatcher. This gets handled in appStore.js
• AppStore then updates the application state
• main.js should be binding the appState to this control as a property. Here's an example with a different UI control
• Updating the appState now triggers a re-render

Using the immutable control approach is ideal because components themself don't have state; everything goes through the app store. Even if we insisted on keeping state, it would be better to just change the control to inherit from React.Component

[bsclifton]

@gyandeeps This is a lot to digest and it's perhaps not the most straight-forward; feel free to hit me up on Slack anytime and we'll work it out 😄

[gyandeeps]

@bsclifton I do understand your approach. I dint do that since this tab width change s such a small state change. It has no impact on the other areas of the application.

Will make the change as you suggested.
Side note: I think in some places i have seen similar (to what i have) approaches taken.

[gyandeeps]

@bsclifton Here we need to change the width based on 2 different events. One is when the frame is closed and one onMouseLeave. Any suggestions on what kind of event name you would prefer for latter.

[bsclifton]

@gyandeeps I think onMouseLeave is appropriate 😄

[gyandeeps]

Why I asked was because onMouseLeave event is very generic looking from a appState perspective.
I can make it specific to tab like onMouseLeaveTab, etc

[bsclifton]

@gyandeeps ahh- ok gotcha... so yes- I would say an event name which is specific is great. So perhaps onTabClosed?

[gyandeeps]

based on our slack chat, will use onTabClose and onTabMouseLeave as event names.

[gyandeeps]

@cezaraugusto @bsclifton I have made changes as suggested. Please review the changes and let me know what u guys think.

[bsclifton]

Finally got a chance to go through the code and try this out... well done! 😄 this works great. Thanks! 😄

[bsclifton]

For some weird reason, I'm not seeing a CI build for this? I'm going to try to submit one manually

[bsclifton]

Closing/re-opening re-triggered CI build 😄 If tests look good, I'll merge!

[bsclifton]

Re-ran the tests which failed with CI and all passed except search suggestions (has been failing for a while) and preferences component (the SVG I checked in with #6689 broke this; I'm looking at it now)

Thanks again, @gyandeeps! Merging...