The final diff of NavigationPanel refactoring. NavigationPanel receives a list of items and renders them. AppSwitcher provides a list with available apps.
Depends on D3340
Details
Details
Check if AppSwitcher still works correctly
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- ENG-445
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
Most of the code is just moved from navigation-panel to app-switcher. I'm marking all the code that is more than a simple move. Code which currently is located in navigation-panel is new.
| web/sidebar/app-switcher.react.js | ||
|---|---|---|
| 74–95 | These were rendered by navigation-panel. They we're extracted from there and wrapped by memo. | |
| 97–106 | Here we're constructing a list of navigation items. This is a new code. | |
| web/sidebar/left-layout-aside.css | ||
| 60–65 | We need to have high specificity here, so that this overrides .container colors. Now we apply the classes to li instead of p. | |
| web/sidebar/app-switcher.react.js | ||
|---|---|---|
| 29 | nit / general comment: We use this selector quite a bit, could be worth turning into it's own selector.  | |
| web/sidebar/app-switcher.react.js | ||
|---|---|---|
| 29 | I think there are a couple of selectors that are duplicated throughout the codebase. I created a task to track improving it https://linear.app/comm/issue/ENG-844/extract-commonly-used-selectors-to-a-common-place | |
Comment Actions
Looks good to me, added a few comments inline.
Adding @ashoat as blocking reviewer on this one to be safe since this diff touches a lot.
| web/sidebar/app-switcher.react.js | ||
|---|---|---|
| 22 | nit: maybe mostRecentlyReadThread? | |
| 23 | nit: thoughts on naming this to something like isActiveThreadCurrentlyUnread? | |
| 68 | I think we usually include the name of the variable in the invariant message? | |
| 81 | I address this in a later diff, but we should increase the click area from just the "Chat" text to include the icon and surrounding area. Doesn't need to be addressed in this diff, just wanted to flag it | |
| web/sidebar/navigation-panel.react.js | ||
|---|---|---|
| 11–14 | I think it would be more idiomatic for this to be children, so it would look something like this: <NavigationPanel.Container> <NavigationPanel.Item tab="chat"> {chatNavigationItem}> </NavigationPanel.Item> <NavigationPanel.Item tab="calendar"> {calendarNavigationItem}> </NavigationPanel.Item> </NavigationPanel.Container> | |
| web/sidebar/app-switcher.react.js | ||
|---|---|---|
| 22 | This diff was only moving this code, but I agree with these suggestions - going to open a new diff after this one so that the complexity of this diff doesn't increase | |
| 81 | Thanks for the info. Yeah, I agree it was a good move | |
| web/sidebar/navigation-panel.react.js | ||
| 11–14 | Makes sense - I'll try to modify this diff with the suggested approach | |
| web/sidebar/app-switcher.react.js | ||
|---|---|---|
| 129–135 ↗ | (On Diff #10227) | I've chosen this approach instead of <NavigationPanel.Item> here so that a calendar that is rendered conditionally can be cleanly expressed. But I'm still open to move NavigationPanel.Items to this return - something like in the suggestion |
Comment Actions
Looks great!
| web/sidebar/navigation-panel.react.js | ||
|---|---|---|
| 17 ↗ | (On Diff #10227) | Why is it necessary to wrap in a React.Fragment? |
| web/sidebar/navigation-panel.react.js | ||
|---|---|---|
| 17 ↗ | (On Diff #10227) | Good point. It isn't necessary - will update the diff with return children. |