feat(dialog): implement <dialog> interface #5120

bennypowers · 2021-08-29T08:38:36Z

Pull Request

📖 Description

adds close and cancel events to dialog. See material-components/material-web#1583

🎫 Issues

👩‍💻 Reviewer Notes

I was unable to get the repo up and running due to some mismatched ramda types (see discord chat)

📑 Test Plan

✅ Checklist

General

I have included a change request file using $ yarn change
I have added tests for my changes.
I have tested my changes.
I have updated the project documentation to reflect my changes.
I have read the CONTRIBUTING documentation and followed the standards for this project.

Component-specific

I have added a new component
I have modified an existing component
I have updated the definition file
I have updated the configuration file

⏭ Next Steps

chrisdholt · 2021-09-01T03:54:33Z

packages/web-components/fast-foundation/src/dialog/dialog.ts

@@ -96,6 +96,8 @@ export class Dialog extends FoundationElement {
     */
    public dismiss(): void {
        this.$emit("dismiss");


@bennypowers Thanks for this! @EisenbergEffect thoughts on how we might deprecate the dismiss event on line 98? If we can mark it deprecated somehow we can add a comment above to remove in the next major version.

sounds like a good use for custom-elements.json

@bennypowers Does the custom-elements.json have a way to indicate that something is deprecated? If not, perhaps we just leave the event in there for now with a comment that it's deprecated, and then remove any documentation on this either on our site or the spec. Then, we can stop publishing the event in a future version.

Looking more at the code, I'm inclined to deprecate the dismiss and hide methods, keeping their old event and state behaviors in place. Then add new cancel() and close() methods with the improved event patterns.

there are two options in custom-elements.json

use a non-standard field and consume it in your own tooling. I opened deprecated flag webcomponents/custom-elements-manifest#84 to track the idea in the spec

Prepend **DEPRECATED** to description members

HTMLDialogElement has close(), show(), and showModal()

https://developer.mozilla.org/en-US/docs/Web/API/HTMLDialogElement

Would love this contribution @bennypowers, especially now that Dialog is a priority for Interop2022 - ideally this can set us up for success in just using that element long term! If you run into issues, we'll find a way to unblock you!

Maintainers should please permit CI to run

@bennypowers Thanks for this! @EisenbergEffect thoughts on how we might deprecate the dismiss event on line 98? If we can mark it deprecated somehow we can add a comment above to remove in the next major version.

Also, since this thread started, deprecated got added to the custom-elements manifest schema

The way I've been handling this in patternfly-elements is like so

/** * `<some-element>` * @fires {MyEvent} my-event - when something happens {@deprecated use `your-event`} * @fires {YourEvent} your-event - when something happens */

Although @deprecated is a block-level tag, I scripted my analyzer to pick them up inline anyways

@williamw2 Since you are investigating custom elements manifest, it would be great if we could support the new deprecated feature. It might be good to sync up with @nicholasrice on some of this as well. I think there's a possibility of using the API extractor metadata from our code to generate the custom elements manifest. There was recent work which Nick contributed to, to enable custom documentation tags. I think it's all merged and ready for use now. If we can use that, we can add custom tags for slots, deprecated, etc. and then use the API extractor metadata to generate the manifest from our code.

Yep, docs for that feature are here: https://api-extractor.com/pages/configs/tsdoc_json/

adds `close` and `cancel` events to dialog

bennypowers · 2022-03-08T14:24:08Z

Having rebased and taken a look at the state of the PR, I'm still mostly sure this is code-complete, but found I was unable to run the lint or test steps after an hour of fiddling. I leave that in maintainers hands, and am happy to make any tooling-related fixes necessary.

chrisdholt · 2022-03-08T18:56:48Z

Having rebased and taken a look at the state of the PR, I'm still mostly sure this is code-complete, but found I was unable to run the lint or test steps after an hour of fiddling. I leave that in maintainers hands, and am happy to make any tooling-related fixes necessary.

I'll pull this down today and see if we can't get things running locally. If all looks good and passes here, I don't see an issue. Thanks @bennypowers!

EisenbergEffect · 2022-03-28T17:49:31Z

@chrisdholt I'm assigning this to you so it doesn't get lost in the mix.

chrisdholt · 2022-04-14T03:02:56Z

This is so odd ~~I'm not able to repro the failures locally~~. I'm going to check out this branch and push a PR to see if it still repro's...

chrisdholt

@bennypowers I think the issue here with the tests may be that the button's click event in the "close" test doesn't do anything. So the test is failing accurately - wasDismissed is not being called. I'm not sure why that's causing everything else to fail as well, but it is.

I think given the intent of when this event fires after looking at the Dialog class (when the hide() method is called), adding the suggestion on the above lines should fix the tests. If you want to push that change I think we'll see all green and be good to finally get this in. Thanks for bearing with us on this one :)

packages/web-components/fast-foundation/src/dialog/dialog.spec.ts

Co-authored-by: Chris Holt <[email protected]>

chrisdholt · 2022-04-14T05:31:35Z

🎉 Thanks @bennypowers!

bennypowers · 2022-04-14T05:31:42Z

So much green!

Thanks @chrisdholt for fixing this 🤗

@latest

# Pull Request ## 🤨 Rationale We want to keep our dependencies on FAST up to date. There are a couple of issues that depend on fixes. e.g. #543, #523 ## 👩‍💻 Implementation 1. npm install all of the @microsoft/fast... packages @latest (as of late last week) 2. Fix a test failure in nimble-components drawer.spec.ts. We were firing a custom `cancel` event but [FAST started firing their own](microsoft/fast#5120), resulting in 2 events instead of the expected 1. See discussion on this PR for details. (thanks @msmithNI for investigating!) 3. Fix a type error in nimble-text-area.directive.spec.ts. This was a result of FAST's change to use const objects and type unions instead of enums microsoft/fast#5930 (it made it so the computed type of that field was the specific value of the enum rather than the type of the enum; the fix widens the type). We'll update the rest of the codebase to use this pattern and address #537 in a follow-up PR #546 since it's a pretty large change. ## 🧪 Testing Mostly relying on pipeline with some light manual testing of storybook. ## ✅ Checklist - [x] I have updated the project documentation to reflect my changes or determined no changes are needed.

bennypowers force-pushed the dialog-events branch from b0f9f7a to 8f5be14 Compare August 29, 2021 08:48

chrisdholt reviewed Sep 1, 2021

View reviewed changes

bennypowers force-pushed the dialog-events branch from 585f3c0 to 255a957 Compare March 8, 2022 00:48

feat(dialog): implement <dialog> interface

14d17aa

adds `close` and `cancel` events to dialog

bennypowers force-pushed the dialog-events branch from 255a957 to 14d17aa Compare March 8, 2022 13:25

Change files

c84b19e

bennypowers marked this pull request as ready for review March 8, 2022 14:09

bennypowers requested review from janechu and nicholasrice as code owners March 8, 2022 14:09

chrisdholt and others added 2 commits March 8, 2022 10:58

Merge branch 'master' into dialog-events

a764031

Merge branch 'master' into dialog-events

b8ba416

nicholasrice approved these changes Mar 28, 2022

View reviewed changes

EisenbergEffect assigned chrisdholt Mar 28, 2022

Merge branch 'master' into dialog-events

ca3c8d2

chrisdholt mentioned this pull request Apr 14, 2022

feat(dialog): implement <dialog> interface #5838

Closed

9 tasks

chrisdholt reviewed Apr 14, 2022

View reviewed changes

packages/web-components/fast-foundation/src/dialog/dialog.spec.ts Show resolved Hide resolved

test(dialog): listen for click

163bf7c

Co-authored-by: Chris Holt <[email protected]>

chrisdholt approved these changes Apr 14, 2022

View reviewed changes

chrisdholt merged commit 9f670f6 into microsoft:master Apr 14, 2022

jattasNI mentioned this pull request May 5, 2022

Update FAST dependencies ni/nimble#542

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(dialog): implement <dialog> interface #5120

feat(dialog): implement <dialog> interface #5120

bennypowers commented Aug 29, 2021 •

edited

Loading

chrisdholt Sep 1, 2021

bennypowers Sep 1, 2021

EisenbergEffect Sep 1, 2021

EisenbergEffect Sep 1, 2021

bennypowers Sep 1, 2021

chrisdholt Mar 8, 2022

bennypowers Mar 8, 2022

bennypowers Mar 8, 2022 •

edited

Loading

EisenbergEffect Mar 8, 2022

nicholasrice Mar 10, 2022

bennypowers commented Mar 8, 2022

chrisdholt commented Mar 8, 2022

EisenbergEffect commented Mar 28, 2022

chrisdholt commented Apr 14, 2022 •

edited

Loading

chrisdholt left a comment

chrisdholt commented Apr 14, 2022

bennypowers commented Apr 14, 2022

feat(dialog): implement <dialog> interface #5120

feat(dialog): implement <dialog> interface #5120

Conversation

bennypowers commented Aug 29, 2021 • edited Loading

Pull Request

📖 Description

🎫 Issues

👩‍💻 Reviewer Notes

📑 Test Plan

✅ Checklist

General

Component-specific

⏭ Next Steps

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennypowers Mar 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bennypowers commented Mar 8, 2022

chrisdholt commented Mar 8, 2022

EisenbergEffect commented Mar 28, 2022

chrisdholt commented Apr 14, 2022 • edited Loading

chrisdholt left a comment

Choose a reason for hiding this comment

chrisdholt commented Apr 14, 2022

bennypowers commented Apr 14, 2022

bennypowers commented Aug 29, 2021 •

edited

Loading

bennypowers Mar 8, 2022 •

edited

Loading

chrisdholt commented Apr 14, 2022 •

edited

Loading