Rework Drawer #587

clintcs · 2025-01-07T15:46:29Z

🚀 Description

Drawer currently fires a "close" event on close. Apparently there's now a "toggle" event too—which Chrome recently added support for. So Drawer needs a "toggle" event. But then why would it need a "close" event too? Just for the sake of following native?

I also learned that Popover API elements have showPopover(), hidePopover(), and togglePopover() methods similar to Drawer's show() and close(). If Drawer has a show() method, then should Popover have showPopover(), hidePopover(), and togglePopover()? Or should neither component have those methods because they both have an open attribute?

I ran all of this by @ynotdraw. After some back and forth, we agreed it makes sense to simplify and diverge from native. We do try to follow native, but not slavishly so. Core tends to diverge when it simplifies things for the consumer or cleans things up a lot for us.

So here's what I propose:

We replace Drawer's "close" event with "toggle". "toggle" is more useful given it also fires on open, and dispatching two events when one will suffice will only confuse consumers.
We remove Drawer's show() and close() methods. Rather than offer two ways to open and close Drawer, consumers can use open—which covers the functionality of both methods and is easier to use in templates.

If everything looks good, I'll bring these changes to Modal next—removing Modal's showModal() and close() methods and its "close" event, and adding a "toggle" event and an open attribute.

For consistency, I'll also add a "toggle" event to other components that have an open attribute. I know of at least one case where a consumer has a need for "toggle" event on Dropdown.

📋 Checklist

I have followed the Contributing Guidelines.
I have added tests to cover new or updated functionality.
I have added or updated Storybook stories.
I have localized new strings.
I have followed the ARIA Authoring Practices Guide or met with the Accessibility Team.
I have included a changeset.
I have scheduled a design review.

🔬 How to Test

Navigate to Drawer in Storybook.
Verify the open attribute opens and closes Drawer.
Verify the "toggle" event in the Actions tab.
Verify that Storybook's controls table looks good.

📸 Images/Videos of Functionality

N/A

changeset-bot · 2025-01-07T15:46:34Z

🦋 Changeset detected

Latest commit: d70f5e9

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@crowdstrike/glide-core	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

github-actions · 2025-01-07T16:29:38Z

Preview URL

https://glide-core.crowdstrike-ux.workers.dev/rework-drawer

clintcs · 2025-01-07T18:39:35Z

src/drawer.test.basics.ts

-  );
-});
-
-it('does not add an `aria-label` when `label` is unset', async () => {


Removed and cleaned up a bunch of tests while I was updating some of them.

We've been gradually removing tests like this that merely test that some property is passed down to into the markup. They don't give us much additional confidence, and our test suites would blow in size if were we to consistently test this kind thing.

clintcs · 2025-01-07T18:44:15Z

src/drawer.ts

 * @cssprop [--width] - The width the drawer.
 *
- * @event close
+ * @event toggle


Another inconsistency I noticed is how we document events. Take a look at Toggle. Now check out Accordion.

The @event tag as specified by JSDoc would simply be @event toggle—as it is here. What should we do? I personally favor @event toggle given the event is fully documented in Storybook.

I was hoping eventually we may be able to populate Storybook with some of these JSDoc comments using some existing tools out there; however, I'm not sure if/when we will. I'd be in favor of this approach for now until we figure out if the JSDoc -> storybook doc generation is something we want to do 👍

clintcs · 2025-01-07T18:47:17Z

src/drawer.ts

-      })();
-    }
-  }
+    if (this.#isOpen && hasChanged) {


No change other than the addition of hasChanged and I got rid of the IIFE in favor of thening off of this.#openAnimation?.finished—no reason to indent the entire block for a couple lines, I figured.

clintcs · 2025-01-07T18:48:26Z

src/drawer.test.interactions.ts

+
+GlideCoreDrawer.shadowRootOptions.mode = 'open';
+
+it('opens when animated', async () => {


Moved these tests from drawer.test.basics.ts. Also moved the tests in drawer.test.methods.ts here.

clintcs · 2025-01-07T18:48:55Z

src/drawer.test.focus.ts

+
+GlideCoreDrawer.shadowRootOptions.mode = 'open';
+
+it('focuses itself on open', async () => {


Moved this test from drawer.test.closing.ts.

clintcs · 2025-01-07T18:51:49Z

src/drawer.test.events.ts


 GlideCoreDrawer.shadowRootOptions.mode = 'open';

-it('dispatches a "close" event when the "Escape" key is pressed', async () => {


No need to test if "close" (now "toggle") is dispatched on Escape given the open setter handles dispatching the event. We only need to test:

That the event is dispatched when open is set (below).

That open is set when Escape is pressed (in drawer.test.interactions.ts).

clintcs · 2025-01-07T19:40:14Z

src/drawer.test.interactions.ts

+
+  component.open = true;
+
+  const aside = component.shadowRoot?.querySelector('[data-test="component"]');


Ugh. These [data-test="component"] and .component patterns are pretty awkward in tests, where we almost always have a const component.

ynotdraw · 2025-01-07T20:11:34Z

src/drawer.ts

 * @cssprop [--width] - The width the drawer.
 *
- * @event close
+ * @event toggle


I was hoping eventually we may be able to populate Storybook with some of these JSDoc comments using some existing tools out there; however, I'm not sure if/when we will. I'd be in favor of this approach for now until we figure out if the JSDoc -> storybook doc generation is something we want to do 👍

clintcs · 2025-01-07T20:26:36Z

I was hoping eventually we may be able to populate Storybook with some of these JSDoc comments using some existing tools out there

Me too. I know there's a schema for doing so. Not sure how well supported it is in Storybook and elsewhere or if it'll be worth the work. But hopefully it's something we'll eventually have time to look into.

src/drawer.test.interactions.ts

clintcs · 2025-01-07T21:33:43Z

Pushed a commit with a couple tweaks.

clintcs force-pushed the rework-drawer branch 2 times, most recently from 12888d7 to 80905cb Compare January 7, 2025 16:25

clintcs commented Jan 7, 2025

View reviewed changes

clintcs force-pushed the rework-drawer branch from 80905cb to 73e5f84 Compare January 7, 2025 18:48

clintcs commented Jan 7, 2025

View reviewed changes

clintcs force-pushed the rework-drawer branch 4 times, most recently from bd4059d to 3d4d3ed Compare January 7, 2025 19:37

clintcs commented Jan 7, 2025

View reviewed changes

Rework Drawer

a111e9d

clintcs force-pushed the rework-drawer branch from 3d4d3ed to a111e9d Compare January 7, 2025 19:43

clintcs marked this pull request as ready for review January 7, 2025 19:47

clintcs requested review from danwenzel, dylankcrwd and ynotdraw as code owners January 7, 2025 19:48

clintcs mentioned this pull request Jan 7, 2025

Rework Modal #586

Merged

ynotdraw approved these changes Jan 7, 2025

View reviewed changes

clintcs commented Jan 7, 2025

View reviewed changes

src/drawer.test.interactions.ts Outdated Show resolved Hide resolved

Reflect Drawer's label and pinned attributes

8969f3f

danwenzel approved these changes Jan 8, 2025

View reviewed changes

Update src/drawer.test.interactions.ts

d70f5e9

clintcs merged commit c0e6911 into main Jan 9, 2025
7 checks passed

clintcs deleted the rework-drawer branch January 9, 2025 00:17

github-actions bot mentioned this pull request Jan 9, 2025

Release Preview #581

Merged

This was referenced Jan 9, 2025

Add "toggle" event to Dropdown, Popover, Menu, and Tooltip #589

Merged

Consistently document @event #593

Merged


		GlideCoreDrawer.shadowRootOptions.mode = 'open';

		it('opens when animated', async () => {


		GlideCoreDrawer.shadowRootOptions.mode = 'open';

		it('focuses itself on open', async () => {


		GlideCoreDrawer.shadowRootOptions.mode = 'open';

		it('dispatches a "close" event when the "Escape" key is pressed', async () => {


		component.open = true;

		const aside = component.shadowRoot?.querySelector('[data-test="component"]');

Rework Drawer #587

Rework Drawer #587

Uh oh!

Conversation

clintcs commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🚀 Description

📋 Checklist

🔬 How to Test

📸 Images/Videos of Functionality

Uh oh!

changeset-bot bot commented Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Jan 7, 2025

Preview URL

Uh oh!

clintcs Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintcs Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ynotdraw Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

clintcs Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintcs Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintcs Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

clintcs Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clintcs Jan 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ynotdraw Jan 7, 2025

Choose a reason for hiding this comment

Uh oh!

clintcs commented Jan 7, 2025

Uh oh!

Uh oh!

clintcs commented Jan 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clintcs commented Jan 7, 2025 •

edited

Loading

changeset-bot bot commented Jan 7, 2025 •

edited

Loading

clintcs Jan 7, 2025 •

edited

Loading

clintcs Jan 7, 2025 •

edited

Loading

clintcs Jan 7, 2025 •

edited

Loading

clintcs Jan 7, 2025 •

edited

Loading

clintcs Jan 7, 2025 •

edited

Loading

clintcs Jan 7, 2025 •

edited

Loading