Add new page and connect it to the route so that it is displayed when a navigation item is selected.
Depends on D4093
Details
Details
- Reviewers
• jacek atul • benschac - Commits
- rCOMM2378fa92db13: [web] Add danger zone page
Open danger zone and check if the header is displayed.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- ENG-835
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Minor refactor suggestion, but otherwise looks good!
| web/app.react.js | ||
|---|---|---|
| 152–165 | Wonder if something like this could be cleaner? Pasted below because the "suggestion" diff can be hard to read: let mainContent;
const { tab, settingsSection } = this.props.navInfo;
if (tab === 'calendar') {
mainContent = <Calendar url={this.props.location.pathname} />;
} else if (tab === 'chat') {
mainContent = <Chat />;
} else if (tab === 'apps') {
mainContent = <AppsDirectory />;
} else if (tab === 'settings' && settingsSection === 'account') {
mainContent = <AccountSettings />;
} else if (tab === 'settings' && settingsSection === 'danger-zone') {
mainContent = <DangerZone />;
} | |
| web/app.react.js | ||
|---|---|---|
| 152–165 | For me having nested ifs here increases the readability, because it is really clear which branches are for which tab. When we have flat structure we need to repeat a part of the condition which is less maintainable due to duplication. If you feel strongly, I can change it. But I don't think it is better. | |
| web/app.react.js | ||
|---|---|---|
| 152–165 | Ok yeah that makes sense. Thoughts on destructuring tab and settingsSection from this.props.navInfo to make things a bit less verbose? | |
| web/app.react.js | ||
|---|---|---|
| 152–165 | Destructuring makes sense! | |