feat(react-draggable-dialog): create draggable dialog with handle #78
Conversation
marcosmoura
added
the
@fluentui-contrib/react-draggable-dialog
| @@ -4,6 +4,10 @@ import { DraggableDialog } from './DraggableDialog'; | |||
|
|
|||
| describe('DraggableDialog', () => { | |||
| it('should render', () => { | |||
| render(<DraggableDialog />); | |||
| render( | |||
There was a problem hiding this comment.
wondering what is actual purpose of this test besides not throwing any runtime error ? which is not probably the best test
There was a problem hiding this comment.
This is not supposed to be a full test, but rather a blank file ready to receive the test code. I have them done, but I'm splitting into a separate PR to make this one smaller.
There was a problem hiding this comment.
IMO when adding logic the test should be part of the PR, otherwise we are merging something that we cannot guarantee to work heh.
but I'll leave this up to your judgement , resolving
There was a problem hiding this comment.
unresolving ->
quite a lot logic was added to this PR , It would be great to add these tests as well as part of it. I dont feel comfortable to approve such amount of logic without any automatic verification that it works.
| /** | ||
| * Text to be announced by screen readers when the dialog is dragged. | ||
| */ | ||
| announcements?: { |
There was a problem hiding this comment.
should this land on initial release ? just wondering as we are not sure if this is actually necessary ?
There was a problem hiding this comment.
The partners might not need announcements as an internal requirement, but they should be available and be part of an accessible Drag'n drop solution.
The announcements prop here is necessary, to translate the text that the underlying library outputs.
There was a problem hiding this comment.
right. I dont see any default values for these, in order to enforce a11y experiences should these be required ?
There was a problem hiding this comment.
I don't know if we need it, to be honest. The DndKit library has defaults for the narration, which will happen regardless of the usage of this prop. The value of this prop here is to provide a way to translate those default messages, which are in English. Partners that don't need to fulfil the narration requirement would have to provide with their defaults for those values, which they don't need.
What do you think about this?
There was a problem hiding this comment.
yeah we can keep it as is. I'd consider though to provide those defaults within our codebase to be explicit.
packages/react-draggable-dialog/src/contexts/DraggableDialogContext.ts
Outdated
Show resolved
Hide resolved
packages/react-draggable-dialog/src/contexts/DraggableDialogContext.ts
Outdated
Show resolved
Hide resolved
packages/react-draggable-dialog/src/hooks/useDraggableDialog.ts
Outdated
Show resolved
Hide resolved
...s/react-draggable-dialog/src/components/DraggableDialogHandle/DraggableDialogHandle.types.ts
Outdated
Show resolved
Hide resolved
packages/react-draggable-dialog/src/components/DraggableDialog/utils.ts
Outdated
Show resolved
Hide resolved
packages/react-draggable-dialog/src/components/DraggableDialog/useDraggableDialog.ts
Outdated
Show resolved
Hide resolved
packages/react-draggable-dialog/src/components/DraggableDialog/useDraggableDialog.ts
Outdated
Show resolved
Hide resolved
|
|
||
| const dndAnnouncements = React.useMemo(() => { | ||
| if (!announcements) { | ||
| return; |
There was a problem hiding this comment.
can we change this api to be more explicit ? return null ?
There was a problem hiding this comment.
We can't set as null, because that'll mean that we don't want any accessibility props. undefined will count as "we didn't pass any value".
| } | ||
|
|
||
| return { | ||
| onDragStart: () => announcements.start, |
There was a problem hiding this comment.
this will allow invalid behaviours, if user uses announcements={} , undefined will be passed - nothing will be announced. IMO we should make those start,end props required
There was a problem hiding this comment.
The DndContext has sane defaults for when any values of the announcements props are not provided. I changed the way we create the announcements object, to only include the props that are passed.
There was a problem hiding this comment.
I see . well that's the hit n miss with 3rd party libraries as some/most things are not obvious in a PR. thank you!
packages/react-draggable-dialog/src/components/DraggableDialog/DraggableDialog.tsx
Outdated
Show resolved
Hide resolved
...react-draggable-dialog/src/components/DraggableDialogSurface/DraggableDialogSurface.types.ts
Outdated
Show resolved
Hide resolved
marcosmoura
changed the title
marcosmoura
deleted the
feat/react-draggable-dialog/create-base-components
branch
Creates the base logic for a functional Draggable Dialog.
Tests will be added later to split the logic into two PRs.
The code is large as the components are too coupled tied (because of a shared context), so in case a review gets confusing, I can gladly explain all the bits.