flake8
passed.
JSHint
passed.
Add a dialog component.
Review Request #14054 — Created July 25, 2024 and submitted
This change adds a new dialog component to Ink. It uses the new(ish)
<dialog>element, which comes with a bunch of advantages. It can be
shown as either a modal or non-modal.The dialog has three areas that can be customized: the title, the body,
and actions. Actions can be separated into primary (right-aligned) and
secondary (left-aligned).Actions can be specified either as regular buttons (or other elements),
or as a specialDialogAction. TheDialogActionexists to prevent
boilerplate for common cases, and has two modes: close or callback. If
an action uses a callback and said callback returns a promise, all of
the dialog's actions will be disabled while that promise runs, and the
action that triggered it will be shown as busy.
- Used dialog in storybook.
- Used this from some code in Review Board.
| Summary | ID |
|---|---|
| 1de317270b5a8beea40749113d095841a409cf3a |
| Description | From | Last Updated |
|---|---|---|
|
I feel like this blends far too muhc. We may need to enhance those edges or tone down the backdrop … |
 chipx86
|
|
|
Not seeing any usage of NON_MODAL, and it looks like the only usage of MODAL isn't really needed given the … |
chipx86
|
|
|
I learned this the hard way recently. We actually would want to make sure we've set this in preinitialize or … |
chipx86
|
|
|
Let's pull out this.el. We reference it multiple times. |
chipx86
|
|
|
Do we want to just embed all the above straight into here? Unless we want to hide the actions areas/subareas … |
chipx86
|
|
|
These are missing docs. |
chipx86
|
|
|
This doesn't seem necessary. |
chipx86
|
|
|
Let's document each of these. It's unclear from the enums what they really do. |
chipx86
|
|
|
Can we move these to option-based arguments? We're bound to want to add to this in time (or have subclasses … |
chipx86
|
|
|
Call sites to this are super confusing. Can we move to option-based arguments? |
chipx86
|
|
|
Do we want to handle any cases where this is something other than a DialogActionView? If this is explicitly allowed, … |
chipx86
|
|
|
This is missing docs. |
chipx86
|
|
|
The ordering all throughout this is inconsistent with other components. It should be alphabetical order. I see that it's in … |
chipx86
|
|
|
Missing docs. |
chipx86
|
|
|
These are also missing docs. |
chipx86
|
-
-
I feel like this blends far too muhc. We may need to enhance those edges or tone down the backdrop a bit.
-
Not seeing any usage of
NON_MODAL, and it looks like the only usage ofMODALisn't really needed given the boolean nature of the argument in question. -
I learned this the hard way recently. We actually would want to make sure we've set this in
preinitializeor it won't make it to the element.Order of construction is:
- Set
cid - Call
preinitialize() - Initialize element and set attributes
- Call
initialize().
- Set
-
-
Do we want to just embed all the above straight into here?
Unless we want to hide the actions areas/subareas based on the content.
-
- Commits:
-
Summary ID b5e530cce399dbff1ec8691f522f7c7a127e47a6 298ec0d381688c14c0784f4b2fcf0a3e331f3fdb - Diff:
-
Revision 2 (+1456)
- Added Files:
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Set a default value for the
modalargument to cover the most common case. - Description:
-
This change adds a new dialog component to Ink. It uses the new(ish)
<dialog>element, which comes with a bunch of advantages. It can beshown as either a modal or non-modal. The dialog has three areas that can be customized: the title, the body,
and actions. Actions can be separated into primary (right-aligned) and secondary (left-aligned). - - Testing Done:
- - Used dialog in storybook. - - Used this from some code in Review Board. - Testing Done:
-
+ - Used dialog in storybook.
+ - Used this from some code in Review Board.
- Commits:
-
Summary ID 298ec0d381688c14c0784f4b2fcf0a3e331f3fdb 6e376a9e83eb52922560b6e45122822e5fa3f81b - Diff:
-
Revision 3 (+1456)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Add a new
DialogActionwhich covers common action cases. - Description:
-
This change adds a new dialog component to Ink. It uses the new(ish)
<dialog>element, which comes with a bunch of advantages. It can beshown as either a modal or non-modal. The dialog has three areas that can be customized: the title, the body,
and actions. Actions can be separated into primary (right-aligned) and secondary (left-aligned). + + Actions can be specified either as regular buttons (or other elements),
+ or as a special DialogAction. TheDialogActionexists to prevent+ boilerplate for common cases, and has two modes: close or callback. If + an action uses a callback and said callback returns a promise, all of + the dialog's actions will be disabled while that promise runs, and the + action that triggered it will be shown as busy. - Commits:
-
Summary ID 6e376a9e83eb52922560b6e45122822e5fa3f81b 0855a6917de9d72b1c3407082e5db672ebb9369a - Diff:
-
Revision 4 (+1860)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
- Change Summary:
-
Added a callback-and-close action as well.
- Commits:
-
Summary ID 0855a6917de9d72b1c3407082e5db672ebb9369a e8639dca34d99ae0c040b9bbf3861d2d98e19bd9 - Diff:
-
Revision 5 (+1862)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.
-
-
-
-
Can we move these to option-based arguments? We're bound to want to add to this in time (or have subclasses add to it).
At that point, I'd also prefer just a boolean for the modal support, since the readability issue is covered.
-
-
Do we want to handle any cases where this is something other than a
DialogActionView? If this is explicitly allowed, let's document that. -
-
The ordering all throughout this is inconsistent with other components. It should be alphabetical order. I see that it's in rough structural order, but not all components work that way, so alphabetical provides the best consistency and makes it easier to find the right part.
-
-
- Commits:
-
Summary ID e8639dca34d99ae0c040b9bbf3861d2d98e19bd9 1de317270b5a8beea40749113d095841a409cf3a - Diff:
-
Revision 6 (+1974)
Checks run (2 succeeded)
flake8
passed.
JSHint
passed.