react-dialog: basic implementation #22051

tringakrasniqi · 2022-03-10T15:52:20Z

New Behavior

Basic implementation of the dialog component, this PR includes:

3 sub-component of dialog: DialogHeader, DialogBody, DialogFooter (thus, the high number of files changed)
aria values based on the type property
styling of component & overlay
stories

Related Issue(s)

Fixes #22023

codesandbox-ci · 2022-03-10T15:58:27Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 1adbfcf:

Sandbox	Source
@fluentui/react 8 starter	Configuration
@fluentui/react-components 9 starter	Configuration

fabricteam · 2022-03-10T15:59:05Z

📊 Bundle size report

_{🤖 This report was generated against 8722596e561b57decfe9aa79121db844737102ff}

size-auditor · 2022-03-10T16:06:40Z

Asset size changes

Size Auditor did not detect a change in bundle size for any component!

Baseline commit: 8722596e561b57decfe9aa79121db844737102ff (build)

jurokapsiar · 2022-03-11T06:03:56Z

packages/react-dialog/src/components/Dialog/Dialog.types.ts

+   * Whether the dialog is open or closed.
+   * @defaultvalue undefined
+   */
+  open: boolean;


let's also introduce onOpenChange, similar to Tooltip's visible and its change callback. This will allow usage of full controlled pattern.

jurokapsiar · 2022-03-11T06:05:50Z

packages/react-dialog/src/components/Dialog/useDialog.ts

+      ref: useMergedRefs(ref, contentRef),
+      role: 'dialog',
+      'aria-modal': !isNonModal && true,
+      'aria-label': props['aria-labelledby'],


aria-label and aria-labelledby are different concepts, looks like this should be removed and we should only rely on spreading of all props

We should however make sure that aria-label or aria-labelledby is set per the WAI design pattern: https://www.w3.org/TR/wai-aria-practices/examples/dialog-modal/dialog.html

I'm not entirely sure if this can be done with types (it would be great it possible) but perhaps a runtime warning (non-production) would work

if (!props['aria-label'] && !props['aria-labelledby'] && process.env.NODE_ENV !== 'production') { console.error('please apply aria-label or aria-labelledby to the Dialog') }

jurokapsiar · 2022-03-11T06:09:05Z

packages/react-dialog/src/stories/AlertDialog.stories.tsx

+        Open Dialog
+      </Button>
+
+      <Dialog type="alert" aria-labelledby="dialog-title" open={isOpen} {...props}>


If Dialog has just a short plain text content, there should be aria-describedby pointing to the DialogBody as described in https://www.w3.org/TR/wai-aria-practices/#dialog_modal. We might need to add some guidance regarding this in the best practices and consider adding it to all stories.

jurokapsiar · 2022-03-11T06:09:49Z

packages/react-dialog/src/stories/AlertDialog.stories.tsx

+        Open Dialog
+      </Button>
+
+      <Dialog type="alert" aria-labelledby="dialog-title" open={isOpen} {...props}>


is there a way how we can enforce usage of label? For example changing the prop interface to require either aria-label or aria-labelledby?

jurokapsiar · 2022-03-11T06:11:35Z

packages/react-dialog/src/stories/AlertDialog.stories.tsx

+        onClick={() => {
+          setIsOpen(true);
+        }}
+        aria-haspopup="true"


This can become a frequent bug where people forget to add aria-haspopup. Trigger component would be a good abstraction to use, and it could still be combined with the controlled state that you propose.

Also, aria-haspopup="dialog" might be more accurate

jurokapsiar · 2022-03-11T06:13:09Z

packages/react-dialog/src/components/Dialog/useDialog.ts

+
+  React.useEffect(() => {
+    if (open && contentRef.current) {
+      const firstFocusable = findFirstFocusable(contentRef.current);


we should also allow focusing elements other than the first one. It is relatively common use case. Maybe a prop where you can pass a selector for default focusable.

Is that a useful API ? something very similar can be done with a simple effect that would be more flexible for users because they can watch for other dependencies like waiting for loading state to end

agree - maybe we should add a story for that then

jurokapsiar · 2022-03-11T06:15:29Z

packages/react-dialog/src/components/Dialog/renderDialog.tsx


 /**
 * Render the final JSX of Dialog
 */
 export const renderDialog_unstable = (state: DialogState) => {
  const { slots, slotProps } = getSlots<DialogSlots>(state);
+  const defaultOverlay = state.type !== 'non-modal' && <div aria-hidden="true" className={state.overlayClassName} />;


Do we also plan to add aria-hidden to all DOM subtrees except of the currently open dialog? It was necessary few months ago, not sure if screen reader advanced in this recently and actually treat aria-modal correctly. What would you advise @smhigley , @ling1726

useModalAttributes is used, so tabster will add aria-hidden everywhere

jurokapsiar · 2022-03-11T06:15:57Z

packages/react-dialog/src/components/Dialog/useDialogStyles.ts

+    ...shorthands.margin('auto'),
+    ...shorthands.border('1px', 'solid', tokens.colorTransparentStroke),
+    ...shorthands.overflow('auto'),
+    zIndex: 1300, //TODO: hardcoded z-index value might need to change


should this be a token? @miroslavstastny

Let's just settle with a value of 1 and partners can override it themselves... not sure if we want to start managing z-index values in theme since it has nothing to do with design

can they do it globally for all instances of Dialog? Or will each developer need to add their own style overrides?

ling1726 · 2022-03-11T09:04:33Z

packages/react-dialog/src/components/Dialog/useDialog.ts

+  const isNonModal = type === 'non-modal';
+
+  const contentRef = React.useRef(null);
+  const { modalAttributes } = useModalAttributes({ trapFocus: !isNonModal && true });


There's also another return triggerAttributes. As I mentioned in the spec PR, it's necessary to revert focus when the dialog is dismissed. triggerAttributes can help you do this.

Feel free to propose another way of doing this. I guess you could also store document.activeElement in your first focusable effect. But we should make sure that this behaviour works from the beginning

A trigger pattern might look something like this: https://www.radix-ui.com/docs/primitives/components/alert-dialog

ling1726 · 2022-03-11T09:06:53Z

packages/react-dialog/src/components/Dialog/useDialog.ts

-      ref,
+      ref: useMergedRefs(ref, contentRef),
+      role: 'dialog',
+      'aria-modal': !isNonModal && true,


Suggested change

'aria-modal': !isNonModal && true,

...(!isNonModal && { 'aria-modal': true })

I think it's safer not to include aria-modal at all rather than have it set to false. Some screen readers might disregard the boolean value

ling1726 · 2022-03-11T09:07:29Z

packages/react-dialog/src/components/Dialog/useDialog.ts

-  return {
-    // TODO add appropriate props/defaults
+  const { overlay, open = false, type = 'modal' } = props;
+  const isNonModal = type === 'non-modal';


Suggested change

const isNonModal = type === 'non-modal';

const isModal = type !== 'non-modal';

nit: I think negative flags should be avoided generally

ling1726 · 2022-03-11T09:11:29Z

packages/react-dialog/src/components/Dialog/useDialog.ts

    root: getNativeElementProps('div', {
-      ref,
+      ref: useMergedRefs(ref, contentRef),
+      role: 'dialog',


Suggested change

role: 'dialog',

role: type === 'alert' ? 'alertdialog' : 'dialog',

ling1726 · 2022-03-11T09:11:52Z

packages/react-dialog/src/components/Dialog/useDialog.ts

+  if (type === 'alert') {
+    state.root.role = 'alertdialog';
+  }


nit: I would prefer to hoist this condition direction to when you declare the props

Suggested change

if (type === 'alert') {

state.root.role = 'alertdialog';

}

Suggestion here
https://github.com/microsoft/fluentui/pull/22051/files#r824521503

ling1726 · 2022-03-11T11:37:49Z

packages/react-dialog/src/components/Dialog/renderDialog.tsx

+  return state.open ? (
+    <Portal>
+      {slots.overlay ? <slots.overlay {...slotProps.overlay} /> : defaultOverlay}
+      <slots.root {...slotProps.root}>{slotProps.root.children} </slots.root>
+    </Portal>
+  ) : null;


Suggested change

return state.open ? (

<Portal>

{slots.overlay ? <slots.overlay {...slotProps.overlay} /> : defaultOverlay}

<slots.root {...slotProps.root}>{slotProps.root.children} </slots.root>

</Portal>

) : null;

if (!state.open) {

return null;

}

return (

<Portal>

{slots.overlay ? <slots.overlay {...slotProps.overlay} /> : defaultOverlay}

<slots.root {...slotProps.root}>{slotProps.root.children} </slots.root>

</Portal>

) ;

nit: I think this is cleaner

msft-fluent-ui-bot · 2022-08-08T18:00:03Z

Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes.

The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code.

tringakrasniqi added 10 commits March 8, 2022 13:40

Scaffold of dialog header component

0c7084e

Scaffold of DialogBody component

c26ce5d

Scaffold of DialogFooter component

387b547

Remove unnecessary stories files

4136119

Added basic impl, styling and story

3ca08df

Added overlay and trapfocus

9eafe9d

Added changes for non-modal and fixed dialog positioning

24071d5

Added styling for 400 zoom

b3580bf

Updated the open prop and aria lables for stories

84cc05b

Typo: uncommented storybook view param

1adbfcf

tringakrasniqi added Component: Dialog Fluent UI react-components (v9) labels Mar 10, 2022

tringakrasniqi added this to the March Project Cycle Q1 2022 milestone Mar 10, 2022

jurokapsiar reviewed Mar 11, 2022

View reviewed changes

ling1726 reviewed Mar 11, 2022

View reviewed changes

msft-fluent-ui-bot added the Resolution: Soft Close Soft closing inactive issues over a certain period label Aug 8, 2022

msft-fluent-ui-bot closed this Aug 8, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

react-dialog: basic implementation #22051

react-dialog: basic implementation #22051

tringakrasniqi commented Mar 10, 2022

codesandbox-ci bot commented Mar 10, 2022

fabricteam commented Mar 10, 2022

size-auditor bot commented Mar 10, 2022

jurokapsiar Mar 11, 2022

jurokapsiar Mar 11, 2022

ling1726 Mar 11, 2022

jurokapsiar Mar 11, 2022

jurokapsiar Mar 11, 2022

jurokapsiar Mar 11, 2022

jurokapsiar Mar 11, 2022

jurokapsiar Mar 11, 2022

ling1726 Mar 11, 2022

jurokapsiar Mar 11, 2022

jurokapsiar Mar 11, 2022

ling1726 Mar 11, 2022

jurokapsiar Mar 11, 2022

ling1726 Mar 11, 2022

jurokapsiar Mar 11, 2022

ling1726 Mar 11, 2022

ling1726 Mar 11, 2022

ling1726 Mar 11, 2022

ling1726 Mar 11, 2022

ling1726 Mar 11, 2022

ling1726 Mar 11, 2022

ling1726 Mar 11, 2022

ling1726 Mar 11, 2022

msft-fluent-ui-bot commented Aug 8, 2022

	'aria-modal': !isNonModal && true,
	...(!isNonModal && { 'aria-modal': true })

	const isNonModal = type === 'non-modal';
	const isModal = type !== 'non-modal';

	role: 'dialog',
	role: type === 'alert' ? 'alertdialog' : 'dialog',

react-dialog: basic implementation #22051

react-dialog: basic implementation #22051

Conversation

tringakrasniqi commented Mar 10, 2022

New Behavior

Related Issue(s)

codesandbox-ci bot commented Mar 10, 2022

fabricteam commented Mar 10, 2022

📊 Bundle size report

size-auditor bot commented Mar 10, 2022

Asset size changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msft-fluent-ui-bot commented Aug 8, 2022