[WIP] Refactor tabs#5133
Conversation
jasonLaster
left a comment
There was a problem hiding this comment.
Looking good…
looks like there’s a new empty tabs action
| @@ -0,0 +1,28 @@ | |||
| /* | |||
There was a problem hiding this comment.
missing flow and the copyright comment
|
so much cleaner! |
| @@ -0,0 +1,217 @@ | |||
| import React, { PureComponent } from "react"; | |||
There was a problem hiding this comment.
Flow + copyright header missing.
| } | ||
| }; | ||
|
|
||
| export default class Tab extends PureComponent<Props, State> { |
There was a problem hiding this comment.
I like that tabs are now their own component!
There was a problem hiding this comment.
One thing though. This component takes lots of props that are from Redux but are being passed through in src/components/Editor/Tabs, maybe instead of default exporting the class we bring in redux connect().
| handleClick={onClickClose} | ||
| tooltip={L10N.getStr("sourceTabs.closeTabButtonTooltip")} | ||
| /> | ||
| {tabSources.map((source, index) => { |
There was a problem hiding this comment.
I think this looks a little nicer as
{tabSources.map((source, index) => (
<Tab
key={index}
source={source}
tabSources={tabSources}
selectedSource={selectedSource}
closeTab={closeTab}
togglePrettyPrint={togglePrettyPrint}
showSource={showSource}
sourceTabsMetaData={sourceTabsMetaData}
selectSource={selectSource}
/>
))}| }; | ||
|
|
||
| export default class Tab extends PureComponent<Props, State> { | ||
| constructor() { |
There was a problem hiding this comment.
If we have a constructor you should take the props argument and pass it to super. But in this case since the body is empty save for the super call we should remove the constructor.
|
|
||
| const isPrettySource = isPretty(tabID); | ||
|
|
||
| const closeTabMenuItem = { |
There was a problem hiding this comment.
Feel like these menu items could be moved to a utility file for most of their body, something like
export const tabMenuItems = {
closeTab: {
id: "node-menu-close-tab",
label: L10N.getStr("sourceTabs.closeTab"),
accesskey: L10N.getStr("sourceTabs.closeTab.accesskey"),
disabled: false
},
...
};Then this way when you're building up the list here all you're adding is the click properties and can spread in the hard coded stuff from elsewhere.
There was a problem hiding this comment.
yep - lets do this in a separate PR though to keep this smaller
Associated Issue: #3279
Here's the Pull Request Doc
https://devtools-html.github.io/debugger.html/CONTRIBUTING.html#pull-requests
Summary of Changes
Tab.js