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

[Dialog] Expose Dialog context #1281

Closed
lesha1201 opened this issue Mar 29, 2022 · 7 comments
Closed

[Dialog] Expose Dialog context #1281

lesha1201 opened this issue Mar 29, 2022 · 7 comments

Comments

@lesha1201
Copy link
Contributor

Feature request

Overview

Expose Dialog context so it's possible to use it in components inside DialogProvider.

Who does this impact? Who is this for?

I want to create a custom Dialog Content component which turns off onPointerDownOutside behaviour and adds click listener on Overlay which closes Dialog so Dialog isn't closed when clicking Overlay's scrollbar (#1280).

Raw example:

export function DialogContent(props) {
  const context = useDialogContext(CONTENT_NAME, props.__scopeDialog);

  const onPointerDownOutside = useCallback(event => {
    event.preventDefault();
  }, []);

  const onClickOverlay = useCallback(
    event => {
      // ...implementation
      context.onOpenChange(false);
    },
    [context.onOpenChange],
  );

  return (
    <DialogPortal>
      <DialogOverlay onClick={onClickOverlay}>
        <BaseDialogContent
          {...props}
          onPointerDownOutside={onPointerDownOutside}
        >
          {children}
        </BaseDialogContent>
      </DialogOverlay>
    </DialogPortal>
  );
}
@jjenzz
Copy link
Contributor

jjenzz commented Mar 29, 2022

the components have a controlled api for this (open and onOpenChange props), so for your particular set up you'd need to do something like:

import { useControllableState } from '@radix-ui/react-use-controllable-state'

const CustomContext = React.createContext();

export function Dialog(props) {
  const [open, setOpen] = useControllableState({
    value: props.open,
    defaultValue: props.defaultOpen,
    onChange: props.onOpenChange
  });
  return (
    <CustomContext.Provider value={React.useMemo(() => ({ onOpenChange: setOpen }), [setOpen])}>
      <Dialog {...props} open={open} onOpenChange={setOpen} />
    </CustomContext.Provider>
  );
}

export function DialogContent(props) {
  const context = React.useContext(CustomContext);

  const onPointerDownOutside = useCallback(event => {
    event.preventDefault();
  }, []);

  const onClickOverlay = useCallback(
    event => {
      // ...implementation
      context.onOpenChange(false);
    },
    [context.onOpenChange],
  );

  return (
    <DialogPortal>
      <DialogOverlay onClick={onClickOverlay}>
        <BaseDialogContent
          {...props}
          onPointerDownOutside={onPointerDownOutside}
        >
          {children}
        </BaseDialogContent>
      </DialogOverlay>
    </DialogPortal>
  );
}

@lesha1201
Copy link
Contributor Author

lesha1201 commented Mar 29, 2022

@jjenzz Thank you for the example.

I don't like that it introduces one more context and state. Wouldn't it be easier and better just to expose useDialogContext hook?

@jjenzz
Copy link
Contributor

jjenzz commented Mar 29, 2022

many consumers don't have the same use-case as you so exposing context would increase overhead for the team for a small subset of use-cases.

primitives purposefully have a small api surface to reduce testing overhead and minimise consumer exposure to breaking changes. for example, an onOpenChange method exists today but there's no guarantee it would tomorrow.

ideally you would build your own apis instead of piggy-backing off of radix's internals, so the current api provides the values/handlers for you to achieve the api your product needs.

all that being said, i do think it is bad UX (and potentially an a11y failure) for the dialog to close when clicking the scrollbar so perhaps your request would be best served raised as a bug for the team to solve as part of the primitive. that way you wouldn't need to wire up anything at all.

@lesha1201
Copy link
Contributor Author

@jjenzz From primitive, I expect it to be as much customizable as possible and one of the keys is having access to its internal context. That way it would cover a lot of use-cases (e.g. ability to add lazy-rendering to Dialog.Portal or any other components). It's impossible to predict many use-cases but from primitive I expect it to be a minimal building block for building more complex components that covers user's use-cases.

I understand that users still can wrap it in their own context and sync it with the internal context but I think it unnecessary complicates everything.

Dialog already exposes createDialogScope which I think is something internal and it's not documented anywhere. It could be the same for useDialogContext.

Anyway thank you for the quick replies. I'm going to introduce my own context and wrap in it Dialog for my use-cases. I think it can be closed then.

@jjenzz
Copy link
Contributor

jjenzz commented Mar 30, 2022

I understand that users still can wrap it in their own context and sync it with the internal context but I think it unnecessary complicates everything.

if the available props do not provide enough for you to create your customisations in a way that you consider appropriate, then by all means, keep this open as a request to improve the DX somehow. exposing context isn't the only solution.

Dialog already exposes createDialogScope which I think is something internal and it's not documented anywhere. It could be the same for useDialogContext.

that is reluctantly exposed for scoping. if it wasn't for this issue it wouldn't be exposed at all. we created an RFC for the react team to request that they consider solving this as part of the framework instead.

Anyway thank you for the quick replies. I'm going to introduce my own context and wrap in it Dialog for my use-cases. I think it can be closed then.

my pleasure. i still think it would be helpful to raise a separate bug for the team about your scrollbar case tho. that is likely an a11y issue that the team should fix as part of the primitive.

@lesha1201
Copy link
Contributor Author

@jjenzz,

my pleasure. i still think it would be helpful to raise a separate bug for the team about your scrollbar case tho. that is likely an a11y issue that the team should fix as part of the primitive.

There is already an issue on Github for that #1280

keep this open as a request to improve the DX somehow.

I'm reopening the issue then but feel free to close it when you want.

@lesha1201 lesha1201 reopened this Mar 30, 2022
@benoitgrelard
Copy link
Collaborator

Hi @lesha1201,

I'll close this issue and we'll take a look at #1280

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants