fix(alert-dialog): add Alert Dialog package #3501
Conversation
Tachometer resultsChromeaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
Firefoxaccordion permalink
action-bar permalink
action-button permalink
action-group permalink
action-menu permalink
|
Rajdeepc
changed the title
Yes there is a bug for this. Thanks for noticing this. I will pull back this PR for now since we need to discuss on the approach to this alert dialog in a more simpler way leveraging DialogBase as the base which includes the modals styles and attributes. This will make the alert dialog much more simpler. |
Rajdeepc
added
Needs discussion
Rajdeepc
added
ready-for-review
and removed
Needs discussion
|
@Westbrook @najikahalsema would love to have your thoughts in this PR. I have abstracted the alert dialog logic to only surface the contents of the alert dialog so that this component can be reused in Tray / Coachmark or Popovers. |
There was a problem hiding this comment.
Some smaller things for clean up.
Only big question is re: the API table that is build by extending Dialog. We may be able to suppress some thing there via:
/**
* @private
*/
public override noDivider = false;
But, I'm not sure we've tried that in an extension before....
| '[slot="button"]', | ||
| ]) | ||
| ) { | ||
| export class Dialog extends ObserveSlotPresence(AlertDialog, [ |
There was a problem hiding this comment.
Nice! Make sure we add @spectrum-web-components/alert-dialog to the dependencies here.
There was a problem hiding this comment.
Sure! Will add once alert-dialog is published to the npm registry.
There was a problem hiding this comment.
That's the wrong order of operations. All packages within the monorepo are dependable today, so there is no need to wait.. When we make a release with this change, were we not to include this dependency, users of the Dialog package couldn't update for the fact that the released version of Dialog would technically be broken unless they were to manually add a dependency on Alert Dialog locally.
| public override connectedCallback(): void { | ||
| super.connectedCallback(); | ||
| window.addEventListener( | ||
| 'resize', | ||
| this.shouldManageTabOrderForScrolling | ||
| ); | ||
| } | ||
|
|
||
| public override disconnectedCallback(): void { | ||
| window.removeEventListener( | ||
| 'resize', | ||
| this.shouldManageTabOrderForScrolling | ||
| ); | ||
| super.disconnectedCallback(); | ||
| } |
There was a problem hiding this comment.
Could I get you to volunteer for a follow up change that moved this to a Resize Observer so that we can also fix #3613?
Westbrook
changed the title
packages/dialog/package.json
Outdated
| @@ -73,6 +73,7 @@ | |||
| "lit-html" | |||
| ], | |||
| "dependencies": { | |||
| "@spectrum-web-components/alert-dialog": "0.0.1", | |||
There was a problem hiding this comment.
| "@spectrum-web-components/alert-dialog": "0.0.1", | |
| "@spectrum-web-components/alert-dialog": "^0.0.1", |
That or it won't get managed correctly at release time.
* chore: first commit for alertdialog * chore: added config for alert dialog * chore: added spectrum-config class attributes * chore: added alert dialog variants * chore: added alert dialog variants * chore: updated variants * chore: added new alertdialog version * chore: removed dialog dependency * chore: added button variant * chore: added storybook * chore: added storybook for scroll variant * chore: added variant tests * chore: updated hash * chore: reverting unused code * chore: bump dependencies version * chore: alert dialog alignment issue * chore: updated golden hash * test: testing * chore: new golden hash * fix: test and css * fix: add golden hash for VRT * chore: removed sp dialog with alert dialog base * fix: css on scroll * fix: updated test case * fix: import fix * fix: sp alert dialog base update in storybook * chore: update icon package for alert icon css * fix: centering issue fix from modal * chore: update golden image hash * chore: update golden image hash * fix: updated tests * chore: update golden hash * test: updated test cases * chore: removed unused imports * chore: adding yarn.lock * fix: updated stories * chore: bump tokens version in styles * chore: update golden image hash * chore: update golden image hash * chore: updated alert dialog wrapper * chore: updated tests * fix: overlay trigger attribute fixes after update * fix: updated version in package.json * fix: updated tokens version * fix: updated tokens version * fix: added open attribute in sp-alert-dialog-wrapper * chore: adding tokens css * chore: golden hash * chore: updated storybook * chore: updated storybook * test: secondary test case added for buttons * fix: css changes in alert dialog, button-group and icon * fix: updated content in readme and toggle button text * chore: updated golden hash * fix: update golden hash * chore: removed alert wrapper * chore: update alert dialog dependency * test: updated alert dialog unit tests * chore: updated golden image cache * chore: removed scroll variant and update readme * chore: inverting the class hierarchy * chore: removed scroll variant * chore: update golden image cache * chore: added alert dialog dependency in dialog * chore: deps update @spectrum-web-components/alert-dialog to ^0.0.1 --------- Co-authored-by: Rajdeep Chandra <[email protected]>
Description
New Alert Dialog Component
Related issue(s)
Motivation and context
How has this been tested?
Screenshots (if appropriate)
Types of changes
Checklist
Best practices
This repository uses conventional commit syntax for each commit message; note that the GitHub UI does not use this by default so be cautious when accepting suggested changes. Avoid the "Update branch" button on the pull request and opt instead for rebasing your branch against
main.