Component-based toolbar customisation API #3067
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
SomeHats
had a problem deploying
to
deploy-staging
GitHub Actions
Failure
SomeHats
had a problem deploying
to
deploy-staging
GitHub Actions
Failure
SomeHats
had a problem deploying
to
deploy-staging
GitHub Actions
Failure
|
First thought before jumping in, I thought we could use two groups here rather than one: 
My hunch was that we could remove a lot of the complexity while still keeping the behavior that we want and keeping the toolbar pretty open to customization. This would remove a lot of the complexity here (including the CSS manipulation, which makes me a little nervous). function DefaultToolbar() {
return (
<div>
<MainToolbarButtons><ToolbarContent/></MainToolbarButtons>
<OverflowToolbarButtons><ToolbarContent/></OverflowToolbarButtons>
</div>
}I also would like us to use the same menu concepts as elsewhere, and I think we could more easily if those two things were managed independently. |
|
Happy to talk through what you're unsure about with the CSS - afaict, there's not a way of avoiding doing something DOM-based like this if we want to drive the API through react components rather than e.g. an For me, the main thing I want to avoid is making consumers have to manage the overflow logic themselves if they want to do custom tools. I don't understand how the two separate groups thing avoids that - the overflow group needs to know how many items are visible in the main group, right? I'm probably missing something from yr description tho |
I think no, it would just use the opposite logic as relates to the breakpoint! i.e. the toolbar might have "show only the first five at breakpoint 4" and the overflow menu would have "show all but the first five at breakpoint 4". |
|
I have a branch that works for what I expected it to work for, but the "last selected shape" is beyond me. We can review together to see if you have any ideas, otherwise I think your approach is the best option. |
|
|
||
| export function OverflowingToolbar({ children }: { children: React.ReactNode }) { | ||
| const editor = useEditor() | ||
| const id = useId().replace(/:/g, '_') |
There was a problem hiding this comment.
why the replacement here? i see we do it in ArrowShapeUtil - should there be a util function safeId or something like that? hard to know why this is necessary on first read
| </div> | ||
| {totalItems > overflowIndex && ( | ||
| <IsInOverflowContext.Provider value={true}> | ||
| <TldrawUiDropdownMenuRoot id="toolbar overflow" modal={false}> |
There was a problem hiding this comment.
i know we have this in a couple places in the codebase but it feels odd to have a space in the id - i know you're not doing this but if there was ever a css selector rule targeting this you wouldn't be able to do it. maybe a bit of pedanticism on my part ¯_(ツ)_/¯
There was a problem hiding this comment.
yeahhh, this is just copied from what was here before. confusingly, this isn't actually a DOM ID at all - it's used to identify this menu in our weird isMenuOpen system
| if (lastActiveOverflowItem) { | ||
| showInMainSelectors.push(`[data-value="${lastActiveOverflowItem}"]`) | ||
| } else { | ||
| showInMainSelectors.push(`:nth-child(${overflowIndex + 1})`) |
There was a problem hiding this comment.
i'm curious: is there a way to do this without the dynamic css?
like, can there be a demarcated element with a className that looks at a boundary or some other kind grouping?
There was a problem hiding this comment.
i don't think so? steve's branch was trying that - we can sort of make it work for truncating the list, but not really for the active item which is pretty dynamic. open to suggestions tho!
| if (!mainToolsRef.current) return | ||
|
|
||
| const mutationObserver = new MutationObserver(onDomUpdate) | ||
| mutationObserver.observe(mainToolsRef.current, { |
There was a problem hiding this comment.
i'm obligated to put this here:
SomeHats
force-pushed
the
alex/toolbar-api
branch
from
a9adbb9
to
2dacf12
Compare
When we went from overrides-based to component based UI customisation APIs, we didn't do the toolbar because it had some significant extra complexity around overflowing the contents of the menu into the dropdown. This is really hard to do at render-time with react - you can't introspect what a component will return to move some of it into an overflow.
Instead, this diff runs that logic in a
useLayoutEffect- we render all the items into both the main toolbar and the overflow menu, then in the effect (or if the rendered components change) we use CSS to remove the items we don't need, check which was last active, etc. Originally, I wasn't really into this approach - but i've actually found it to work super well and be very reliable.Change Type
major— Breaking changedependencies— Changes to package dependencies1documentation— Changes to the documentation only2tests— Changes to any test code only2internal— Any other changes that don't affect the published package2Test Plan
Footnotes
publishes a
patchrelease, for devDependencies useinternal↩will not publish a new version ↩ ↩2 ↩3