Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Toolbar] POC with context checking API #1437

Closed
wants to merge 17 commits into from

Conversation

mj12albert
Copy link
Member

@mj12albert mj12albert commented Feb 11, 2025

Demo: https://deploy-preview-1437--base-ui.netlify.app/experiments/toolbar/text-editor

<Toolbar.Root>
  <Select.Root>
    <Select.Trigger />
    <Select.Portal>
     {/* the rest of the select */}
    </Select.Portal>
  </Select.Root>
  <Toolbar.Separator />
  <ToggleGroup>
    <Toggle value="bold" />
    <Toggle value="italics" />
    <Toggle value="underline" />
  </ToggleGroup>
  <Menu.Root>
    <Menu.Trigger />
    <Menu.Portal>
     {/* the rest of the menu */}
    </Menu.Portal>
  </Menu.Root>
</Toolbar.Root>

Full source: https://github.com/mui/base-ui/blob/2eae6834348d70804ca94c93f97c04b2f4738adf/docs/src/app/(private)/experiments/toolbar/text-editor.tsx

Forked from #1349

@mj12albert mj12albert added component: toolbar The React component. proof of concept Studying and/or experimenting with a to be validated approach labels Feb 11, 2025
Copy link

netlify bot commented Feb 11, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit 2eae683
🔍 Latest deploy log https://app.netlify.com/sites/base-ui/deploys/67ab016c4dfe8b0008bdbaa4
😎 Deploy Preview https://deploy-preview-1437--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@mj12albert
Copy link
Member Author

mj12albert commented Feb 11, 2025

Issues so far:

  1. Seems impossible for TS to enforce the focusableWhenDisabled prop on various triggers depending on whether it's placed in a Toolbar or not
    • Input and NumberField can be toolbar items, focusableWhenDisabled will be exposed on these too?
  2. Manually merging the disabled states is tricky, mergeReactProps could do it automatically but this API intentionally wants to bypass it (also requires extra code). In the demo, if the MenuTrigger is disabled (use the settings panel) aria-disabled/data-disabled are not applied correctly right now even though the trigger is functionally disabled
  3. Menu/Select trigger intentionally sets tabIndex, if they are a CompositeItem, that tabIndex needs to override the one on the trigger, resulting in the same issue as 2

2/3 could possibly be worked around by not using <CompositeItem /> and using useCompositeItem directly, though it will result in more duplication

Not sure if anything can be done about 1, it doesn't seem ideal to expose focuseableWhenDisabled on regular triggers

import { useToolbarRootContext, type ToolbarRootContext } from './ToolbarRootContext';
import type { ToolbarItemMetadata } from './ToolbarRoot';

function useCompositeButton(parameters: useCompositeButton.Parameters) {
Copy link
Member Author

@mj12albert mj12albert Feb 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this is super rough right now, don't look too closely)

@mj12albert mj12albert changed the title POC Toolbar with Context API [Toolbar] POC with Context API Feb 11, 2025
@mj12albert mj12albert changed the title [Toolbar] POC with Context API [Toolbar] POC with context checking API Feb 11, 2025
@mj12albert
Copy link
Member Author

Going with the original API in #661

@mj12albert mj12albert closed this Feb 17, 2025
@mj12albert mj12albert deleted the poc/toolbar-context branch February 17, 2025 05:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: toolbar The React component. proof of concept Studying and/or experimenting with a to be validated approach
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant