Hide the active workspace when moving a folder #2853
Conversation
| {workspaces.flatMap(w => | ||
| w._id === workspace._id ? ( | ||
| [] | ||
| ) : ( | ||
| <option key={w._id} value={w._id}> | ||
| {w.name} | ||
| </option> | ||
| ), | ||
| )} |
There was a problem hiding this comment.
I'm guessing you didn't go down the workspaces.filter(...).map(...) route to avoid iterating through the array twice:
Is there a particular reason to use flatMap over map, though? If you use a map, you can simply return null in place of an empty array in the conditional.
There was a problem hiding this comment.
Is there a particular reason to use
flatMapovermap, though?
Not really other than personal preference. I don't like using null when I can avoid it and since it's already an array of workspaces here it seems more clear to provide an array. Performance wise it's nearly identical:
Edit: If you prefer a map I can update it.
There was a problem hiding this comment.
That's good to know! I don't really have a preference, I'm on board with avoiding null where possible, and I can see cases where flatMap will certainly be safer.
I feel that for the sake of consistency in this code-base (only one instance of flatMap), and the fact that React knows exactly what to do with a null, in this particular instance it might be better to stick with map.
Btw, what did you use to get that performance snapshot? Looks useful!
There was a problem hiding this comment.
Good point about React knowing what to do with null for the conditional rendering. I didn't check to see if flatMap had been used elsewhere in the project and this being the only instance would be annoying for maintenance. I've updated the PR with this change. If things look good after that I'll squash the 3 commits in to one.
Btw, what did you use to get that performance snapshot? Looks useful!
It's https://jsbench.me, I wouldn't use it for anything large, but it's great for testing in these sorts of scenarios.
There was a problem hiding this comment.
We squash and merge PRs through GH, so no need to squash into a single commit
| @@ -12,6 +12,7 @@ import HelpTooltip from '../help-tooltip'; | |||
|
|
|||
| type Props = { | |||
| workspaces: Array<Workspace>, | |||
| workspace: Workspace, | |||
There was a problem hiding this comment.
Please change this prop to be activeWorkspace to be clearer about what it represents.
There was a problem hiding this comment.
I considered this when making the PR but wasn't sure since it was labeled workspace in other instances, thanks for the clarification. This is now fixed for all references.
There was a problem hiding this comment.
Oh, you're right, it is labeled simply workspace in other places. I don't know why... since it loses context. I'm not too concerned, but I think keeping context about what the workspace represents is useful for this particular case!
develohpanda
changed the title
|
Did a quick merge in of the develop branch to get back up to date since I wasn't sure if you prefer that or a rebase since it will be squashed any way. |
Closes #2849
Before:
After:
This uses a
flatMapwhich should have minimal impact on performance. I didn't see any tests for this specific component. If I missed them let me know and I can add some.