Skip to content

feat(react-dialog): Dialog initial implementation #23933

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.

Sign up for GitHub

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

Jump to bottom
Merged
bsunderhus merged 8 commits into from
Jul 20, 2022
Merged

feat(react-dialog): Dialog initial implementation #23933

bsunderhus merged 8 commits into from
Jul 20, 2022

Conversation

@bsunderhus
Copy link
Contributor

@bsunderhus bsunderhus commented Jul 15, 2022 •
edited
Loading

Provides initial implementation for the Dialog component

@bsunderhus bsunderhus self-assigned this Jul 15, 2022
@github-actions github-actions bot added this to the July Project Cycle Q3 2022 milestone Jul 15, 2022
@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 15, 2022 •
edited
Loading

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 700b6c1:

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

@bsunderhus bsunderhus force-pushed the dialog-initial-implementation branch 2 times, most recently from ec3e699 to 66c7fe3 Compare July 15, 2022 14:52
@fabricteam
Copy link
Collaborator

fabricteam commented Jul 15, 2022 •
edited
Loading

📊 Bundle size report

Package & Exports Baseline (minified/GZIP) PR Change
react-dialog
Dialog		 0 B
0 B		 50.739 kB
15.524 kB		 🆕 New entry

🤖 This report was generated against bb5ec4f8d3f9891901064e92d81e89c3c8022951

@size-auditor
Copy link

size-auditor bot commented Jul 15, 2022 •
edited
Loading

Asset size changes

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

Baseline commit: bb5ec4f8d3f9891901064e92d81e89c3c8022951 (build)

@bsunderhus bsunderhus force-pushed the dialog-initial-implementation branch 2 times, most recently from 04bfee6 to 8aec66a Compare July 17, 2022 09:17
@fabricteam
Copy link
Collaborator

fabricteam commented Jul 17, 2022

Perf Analysis (@fluentui/react-components)

No significant results to display.

All results

Scenario Render type Master Ticks PR Ticks Iterations Status
Avatar mount 1315 1301 5000
Button mount 912 917 5000
FluentProvider mount 1745 1755 5000
FluentProviderWithTheme mount 609 613 10
FluentProviderWithTheme virtual-rerender 571 578 10
FluentProviderWithTheme virtual-rerender-with-unmount 626 633 10
MakeStyles mount 1970 1912 50000
SpinButton mount 2643 2563 5000

@bsunderhus bsunderhus force-pushed the dialog-initial-implementation branch 3 times, most recently from f768687 to 1bdea0f Compare July 18, 2022 09:44
@bsunderhus bsunderhus marked this pull request as ready for review July 18, 2022 09:57
@bsunderhus bsunderhus requested a review from a team as a code owner July 18, 2022 09:57
@bsunderhus bsunderhus force-pushed the dialog-initial-implementation branch from 1bdea0f to 3f8781d Compare July 18, 2022 10:36
@bsunderhus bsunderhus changed the title Dialog initial implementation feat(react-dialog): Dialog initial implementation Jul 18, 2022
@bsunderhus bsunderhus mentioned this pull request Jul 19, 2022
Closed
33 tasks
@bsunderhus bsunderhus force-pushed the dialog-initial-implementation branch 3 times, most recently from c306410 to 7f0b267 Compare July 19, 2022 10:49
@bsunderhus bsunderhus requested a review from ling1726 July 19, 2022 10:51
@bsunderhus bsunderhus force-pushed the dialog-initial-implementation branch from 7f0b267 to 06e90c4 Compare July 19, 2022 11:24
@bsunderhus bsunderhus force-pushed the dialog-initial-implementation branch from 0755173 to 700b6c1 Compare July 20, 2022 08:03
@bsunderhus bsunderhus requested a review from ling1726 July 20, 2022 08:03
@bsunderhus bsunderhus merged commit db30e4c into microsoft:master Jul 20, 2022
@bsunderhus bsunderhus deleted the dialog-initial-implementation branch July 20, 2022 08:21
layershifter
layershifter reviewed Jul 28, 2022
View reviewed changes
packages/react-components/react-dialog/src/components/Dialog/Dialog.types.ts
Comment on lines +18 to +24
| { type: 'escapeKeyDown'; open: boolean; event: React.KeyboardEvent }
/**
* document escape keydown defers from internal escape keydown events because of the synthetic event API
*/
| { type: 'documentEscapeKeyDown'; open: boolean; event: KeyboardEvent }
| { type: 'overlayClick'; open: boolean; event: React.MouseEvent }
| { type: 'triggerClick'; open: boolean; event: React.MouseEvent };
Copy link
Member

@layershifter layershifter Jul 28, 2022

Choose a reason for hiding this comment

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

Can you please clarify why event is passed in data?

<Dialog onOpenChange={(event, data) => {
  // What `event` should I use?
  console.log(event, data.event)
}} />

IMO this looks redundant and confusing...

layershifter
layershifter reviewed Jul 28, 2022
View reviewed changes
packages/react-components/react-dialog/src/components/Dialog/useDialog.ts
},
});

const requestOpenChange = useEventCallback((data: DialogRequestOpenChangeData) => {
Copy link
Member

@layershifter layershifter Jul 28, 2022

Choose a reason for hiding this comment

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

nit: not a big fun of using "request" prefix as there are clear actions for be executed... Something like toggleOpen/setOpen/dispatchOpen sounds better. But it's all IMO

layershifter
layershifter reviewed Jul 28, 2022
View reviewed changes
packages/react-components/react-dialog/src/components/Dialog/useDialog.ts
});

const requestOpenChange = useEventCallback((data: DialogRequestOpenChangeData) => {
const getNextOpen = normalizeSetStateAction(data.open);
Copy link
Member

@layershifter layershifter Jul 28, 2022

Choose a reason for hiding this comment

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

I briefly checked all existing usages and it looks for that it would be better to pass just a string there: as a single case when it could a function it's a trigger that uses strings...

If you just have there action: 'open' | 'close' | 'toggle' it will be more straightforward IMO

layershifter
layershifter reviewed Jul 28, 2022
View reviewed changes
packages/react-components/react-dialog/src/components/Dialog/useDialog.ts

const requestOpenChange = useEventCallback((data: DialogRequestOpenChangeData) => {
const getNextOpen = normalizeSetStateAction(data.open);
const isDefaultPrevented = normalizeDefaultPrevented(data.event);
Copy link
Member

@layershifter layershifter Jul 28, 2022

Choose a reason for hiding this comment

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

A dumb question, but why it's needed? defaultPrevented exists on React's SyntheticEvent... Why it cannot be simply event.defaultPrevented?

layershifter
layershifter reviewed Jul 28, 2022
View reviewed changes
packages/react-components/react-dialog/src/components/Dialog/useDialog.ts
* Checks if dialog is opening
*/
function isOpening(current: boolean, next: Extract<React.SetStateAction<boolean>, Function>) {
return !current && next(current) === true;
Copy link
Member

@layershifter layershifter Jul 28, 2022

Choose a reason for hiding this comment

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

(☞ﾟ∀ﾟ)☞ return !current && next(current)

(no reason to use === true)

layershifter
layershifter reviewed Jul 28, 2022
View reviewed changes
packages/react-components/react-dialog/src/components/Dialog/useDialog.ts
} else {
if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line no-console
console.warn('A Dialog should have at least one focusable element inside DialogContent');
Copy link
Member

@layershifter layershifter Jul 28, 2022

Choose a reason for hiding this comment

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

nit: But I would think on how to emit this warning once to avoid spam in console

layershifter
layershifter reviewed Jul 28, 2022
View reviewed changes
packages/react-components/react-dialog/src/components/Dialog/useDialogStyles.ts
// TODO Add default styles for the root element
overlay: {
position: 'fixed',
backgroundColor: 'rgba(0, 0, 0, 0.4)',
Copy link
Member

@layershifter layershifter Jul 28, 2022

Choose a reason for hiding this comment

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

Can we use a token for that?

layershifter
layershifter reviewed Jul 28, 2022
View reviewed changes
packages/react-components/react-dialog/src/components/Dialog/useDialogStyles.ts
export const useDialogStyles_unstable = (state: DialogState): DialogState => {
const styles = useStyles();
state.root.className = mergeClasses(dialogClassNames.root, styles.root, state.root.className);
const isNestedDialog = useHasParentContext(DialogContext);
Copy link
Member

@layershifter layershifter Jul 28, 2022

Choose a reason for hiding this comment

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

I suggest to move this to state otherwise it will not be accessible as a value for composition and cannot be used on consumer side

layershifter
layershifter reviewed Jul 28, 2022
View reviewed changes
packages/react-components/react-dialog/src/contexts/dialogContext.mock.ts
* Don't forget to call `jest.mock(**\/dialogContext.ts)` while using this
* @param options Menu context values to set for testing
*/
export const mockUseDialogContext = (options: Partial<DialogContextValue> = {}) => {
Copy link
Member

@layershifter layershifter Jul 28, 2022

Choose a reason for hiding this comment

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

I would avoid mocks if possible. I would rather use a mocked Provider in tests with wrapper

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@layershifter layershifter layershifter left review comments

@miroslavstastny miroslavstastny Awaiting requested review from miroslavstastny

+1 more reviewer

@ling1726 ling1726 ling1726 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@bsunderhus bsunderhus

Labels

Component: Dialog Fluent UI vNext

Projects

None yet

Milestone

July Project Cycle Q3 2022

Development

Successfully merging this pull request may close these issues.

5 participants

@bsunderhus @fabricteam @miroslavstastny @layershifter @ling1726