feat: Editor Menu #3028
feat: Editor Menu #3028
Conversation
|
Removed vultr server and associated DNS entries |
ianjon3s
force-pushed
the
ian/editor-sidebar-menu
branch
2 times, most recently
from
d04e959 to
12b0ab0
Compare
DafyddLlyr
force-pushed
the
ian/editor-sidebar-menu
branch
from
6358329 to
a079028
Compare
| @@ -17,37 +17,33 @@ const TestEnvironmentWarning = styled(Box)(({ theme }) => ({ | |||
| })); | |||
|
|
|||
| const TestEnvironmentBanner: React.FC = () => { | |||
| const [showWarning, setShowWarning] = useState(true); | |||
| const [isTestEnvBannerVisible, hideTestEnvBanner] = useStore(state => [state.isTestEnvBannerVisible, state.hideTestEnvBanner]); | |||
There was a problem hiding this comment.
This is now controlled at a store level as it was being re-rendered (and therefore going back to visible) on each route change.
I think this is the same issue we're seeing with everything re-rendering when we got to /nodes routes. See comment below on this 👍
|
|
||
| const isActive = (route: string) => lastChunk.url.pathname.endsWith(route); | ||
|
|
||
| const routes = [ |
There was a problem hiding this comment.
@ianjon3s This pattern of splitting up "content" and "code" is a good one to be aware of.
We now have a list of items we're iterating over with the .map() below.
It's now a bit simpler to add or remove a menu item, or control the visual appearance of all of them consistently 👍
| const [flow, ...breadcrumbs] = req.params.flow.split(","); | ||
| return ( | ||
| <> | ||
| <FlowEditor key={flow} flow={flow} breadcrumbs={breadcrumbs} /> |
There was a problem hiding this comment.
This duplication of FlowEditor is worth being aware of. It matches the current setup where this is attached the view and duplicated that way (just a <span/> is rendered in the current setup.
It's the cause of our re-rendering when we go to /node routes - react-navi is re-rendering all other views, even those unaffected for some reason.
I repeated previous attempts with memoization here to attempt to solve this with the same results as before.
The Header is also re-rendering when children change, so this may be a wider (or higher level) issue. Happy to carry on looking into it without holding up this PR.
| })), | ||
| ), | ||
|
|
||
| "/submissions-log": compose( |
There was a problem hiding this comment.
I've not hidden these routes behind a feature flag as there's nothing sensitive or different here, and they've not very discoverable.
ianjon3s
force-pushed
the
ian/editor-sidebar-menu
branch
from
26a8058 to
29b329f
Compare
There was a problem hiding this comment.
Functionally this works as expected for me! Thanks especially for tweaking some component heirarchies and starting to make a dent in the unexpected re-renders.
Based on level of surprise/confusion this week to new editor styles, merging behind a feature flag definitely feels right for now.
For me, the left menu icons are visually competing a bit with the icons/buttons at the top of the right sidebar now, not sure it feels intuitive yet where I find each type of thing (eg submissions log on left but analytics on right?). One to easily revisit in a followup PR & keep adjusting behind feature flag tho 🙂
|
Thanks for the review! Agreed that these changes are probably best released as part of a wider refresh with a bit of communication around it 👍 |
Screen.Recording.2024-04-26.at.14.50.12.mov
What does this PR do?
EditorMenu(behind a feature flag)Next steps