Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(react-draggable-dialog): create draggable dialog with handle #78
feat(react-draggable-dialog): create draggable dialog with handle #78
Changes from 5 commits
119914a
b9769ea
ccb0866
f4cde2a
0b5be5e
7616f31
cc33e33
def9b7d
41d129b
d583361
af40f60
efc4535
67397b0
a76564a
765c65e
ae92b6f
225a6bb
db4cd0f
12589f3
79b917c
f4ce87a
3a0d0ca
037e788
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we change this api to be more explicit ? return
null
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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".There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 requiredThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The
DndContext
has sane defaults for when any values of theannouncements
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah we can keep it as is. I'd consider though to provide those defaults within our codebase to be explicit.