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
Components: Add experimental
ConfirmDialog
#34153Components: Add experimental
ConfirmDialog
#34153Changes from 50 commits
72cd96b
335053a
f4a3396
1b2f104
d5e8d73
f7250b6
6062648
a01225f
a4d2453
18e0231
2391e35
a878802
b09651b
c221e9d
cac2732
0942707
7d629b8
12f0b18
f99c357
f746119
5fd979f
13bf86a
d68508f
56ea610
6245385
814fc6f
2b47a46
bf9c062
37befa2
31b88e2
2cd86e9
21600b0
b21c175
922c103
9c931c6
5494585
2c43770
fdab69f
835c2e6
61f7c1a
c1cf65a
20e2d23
33e97e1
566fe18
d1be655
81d9e76
8665ad6
29c8c3a
f59553d
55ac46a
c96f575
3abfc81
605c2ae
6a60f92
995f45f
40cb594
ceabb07
aa7025d
d130ed1
054caae
85e9510
c20f85d
e9ee813
316afc5
5b2b026
4e5abed
9bf6f88
64c2816
414ab15
49eaa0e
9f62ad3
af340b2
710932d
5e24ff0
7d5860c
febda4a
8bed797
16ce455
2ddbabc
bd6eb33
94f8b47
4d35f84
74c5293
f69a973
541490c
8eeb0c7
3c3f49a
173f2d4
4a81ba6
e2ce3ef
c2a9adb
a320d85
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Is there any way we can improve this situation, so that we don't have conditionally required props? Maybe the best solution is to make it always required?
Or to expose two different components, one controlled and one uncontrolled? @diegohaz what do you think?
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 could also try experimenting with TypeScript types, something like
Not sure.
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, this is the part I want the most feedback on. I came up with this API as I built the component to look and behave like the native
confirm
component (and be easy and straightforward to use), while still allowing it to be used as a controlled component. I think it mainly depends on how we plan on consuming this component: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.
Hey @fullofcaffeine , sorry I haven't had much time to follow up on this. Have you made any changes to how the controlled vs uncontrolled components are exposed, or to how the variables are typed?
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 comment may actually include a good solution for typing 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.
I wonder what happens if:
isOpenProp
is set tofalse
— the dialog is not shownisOpenProp
is unset — would the dialog suddenly show?Definitely somewhat of an edge case, can be tackled in a follow-up PR
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.
Hmm, not sure. I can add a test case for that in a follow-up PR, I'll take note!
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.
Minor, but this could probably be memoised with the
useCallback
hookThere 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.
Do you think we should allow users of this component to change the
Cancel
andOK
text? We could potentially expose them asstring
props, something likeconfirmLabel
andcancelLabel
?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 first version was meant to be a (mostly) 1:1 model of
confirm
, hence the hardcoded labels, but yep! @jasmussen also suggested it in his comment above.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'm considering working on that in a follow-up PR, and include a more detailed review / discussion of the visual design and UX aspects. What do you think? cc @ciampo @jasmussen
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.
If the design still looks like #34153 (comment), I think we need to at least replace the secondary style cancel button with a tertiary style.
The title wrapping two lines is a bit challenging. The default confirm dialog shows the "Would you like to privately publish this post now?" as just text, not a heading. Can we remove the heading unless explicitly set? I.e. something like:
We do have a little wiggleroom to keep PRs small, but these would be nice changes to get along.
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.
@jasmussen That's great feedback, thanks! I'll include those changes as part of this changeset.
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.
@jasmussen I've implemented the changes you suggested, you can see all scenarios on the storybook for
ConfirmDialog
, here's a video:Peek.2021-10-07.18-57.mp4
I could use some feedback regarding the standardization of CSS rules' values. I see some older non-G2 components use some standardized (and sometimes dynamically-calculated) values that are set to SASS variables. I also found what seems to be the equivalent for emotion but not sure what / when to use each. For now, I just hardcoded to good old pixel units. More details here.
cc @ciampo @diegohaz
EDIT: I just noticed I misspelled "privately" in the screencast above as "privetely". Already fixed in the code.
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.
Is this necessary, even if we use the
Modal
component (which I assumed already had styles to render on top of every other element)?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.
From my tests with the
post-visibiliy
popover, it is. I'll double-check now and let you know.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.
You can definitely use variables from the configuration that you linked. By having a quick look at it, though, I don't see a value that you could use for the
padding-top
rule.I think your best option would be to use the
space
util directly. It is based on a grid of4px
, which is great in your case since you have24px
and20px
values to replace.Here's what you could do
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 should never rely on hardcoded classnames when styling our components. The correct solution here would be to improve the Modal component to:
We should at least add a comment saying that we should refactor away from hardcoded classnames ASAP by improving the
Modal
component.See also a similar conversation on the
ToolsPanel
component#35415 (comment)