feat(Drawer): add `open` attribute #497

dylankcrwd · 2024-12-02T19:39:59Z

🚀 Description

Adds an open attribute to the drawer component.

Resolves an issue in main where, in the drawer story, the "toggle" button must be clicked twice in order to re-open the drawer after pressing the escape key.

📋 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

To test the open attribute and property:

In Storybook, navigate to the Drawer story.
Select the glide-core-drawer component and add the open attribute. Observe that the drawer opens.
Continuing on from step (2), remove the open attribute and observe that the drawer closes.
In the console, set the glide-core-drawer open property to true and observe that the drawer opens.
Continuing on from step (4), set the glide-core-drawer open property to false and observe that the drawer closes.

To test the escape key issue in the story:

In Storybook, navigate to the Drawer story and open it.
Press escape, allowing for the drawer to close, then press the "toggle" button once. Observe that the drawer opens without needing to press the button twice (as is the case in main).

📸 Images/Videos of Functionality

open attribute:

Screen.Recording.2024-12-02.at.3.46.57.PM.mov

Storybook escape issue before:

Screen.Recording.2024-12-02.at.3.49.47.PM.mov

Storybook after:

Screen.Recording.2024-12-02.at.3.57.04.PM.mov

changeset-bot · 2024-12-02T19:40:02Z

🦋 Changeset detected

Latest commit: 74c65c2

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	Patch

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 · 2024-12-02T19:43:47Z

Preview URL

https://glide-core.crowdstrike-ux.workers.dev/add-drawer-open-attribute

ynotdraw

Screen.Recording.2024-12-02.at.7.06.26.PM.mov

Code looks good to me, but I seem to find myself in a weird spot if I spam the open attribute toggle. I hit this by accident with hitting the spacebar initially and then the Drawer got stuck in this weird position.

Maybe not a real life scenario, but I'm wondering if this is a simple fix? It looks like the aside keeps both the closing open classes on the element no matter the open attribute once it gets in this state, so then it's stuck. What do you think?

ynotdraw · 2024-12-02T23:56:00Z

.changeset/lazy-geese-argue.md

+'@crowdstrike/glide-core': patch
+---
+
+Adds an `open` attribute to Drawer.


Super nit to help with reader/consumer information rather than sounding specific to us:

Suggested change

Adds an `open` attribute to Drawer.

Drawer now supports an `open` attribute.

ynotdraw · 2024-12-02T23:58:07Z

src/drawer.test.basics.ts

+    html`<glide-core-drawer>Drawer content</glide-core-drawer>`,
+  );
+
+  expect(component.shadowRoot?.querySelector('aside[data-test-state="closed"]'))


Super nit, no need to change, but I think you could use the data-test- selector itself and drop the aside if you'd like:

Suggested change

expect(component.shadowRoot?.querySelector('aside[data-test-state="closed"]'))

expect(component.shadowRoot?.querySelector('[data-test-state="closed"]'))

Adding aside is a bit more explicit to make sure the data-test-state is coming from there though 🤷

ynotdraw · 2024-12-02T23:58:16Z

src/drawer.test.basics.ts

+
+  await elementUpdated(component);
+
+  expect(component.shadowRoot?.querySelector('aside[data-test-state="open"]'))


Similar nit to above:

Suggested change

expect(component.shadowRoot?.querySelector('aside[data-test-state="open"]'))

expect(component.shadowRoot?.querySelector('[data-test-state="open"]'))

ynotdraw · 2024-12-02T23:58:22Z

src/drawer.test.basics.ts

+    html`<glide-core-drawer open>Drawer content</glide-core-drawer>`,
+  );
+
+  expect(component.shadowRoot?.querySelector('aside[data-test-state="open"]'))


Suggested change

expect(component.shadowRoot?.querySelector('aside[data-test-state="open"]'))

expect(component.shadowRoot?.querySelector('[data-test-state="open"]'))

ynotdraw · 2024-12-02T23:58:28Z

src/drawer.test.basics.ts

+
+  await elementUpdated(component);
+
+  expect(component.shadowRoot?.querySelector('aside[data-test-state="closed"]'))


Suggested change

expect(component.shadowRoot?.querySelector('aside[data-test-state="closed"]'))

expect(component.shadowRoot?.querySelector('[data-test-state="closed"]'))

dylankcrwd · 2024-12-03T13:49:02Z

Code looks good to me, but I seem to find myself in a weird spot if I spam the open attribute toggle. I hit this by accident with hitting the spacebar initially and then the Drawer got stuck in this weird position.

Maybe not a real life scenario, but I'm wondering if this is a simple fix? It looks like the aside keeps both the closing open classes on the element no matter the open attribute once it gets in this state, so then it's stuck. What do you think?

I suspect this issue has always been there and that the reason is multiple transistionend events & listeners are fired close in time. The approach to animating this would need to change (e.g. web animation api). I'll look into it.

clintcs · 2024-12-03T14:51:51Z

src/drawer.stories.ts

    pinned: false,
    '--width': '',
+    open: false,


Mind moving open and pinned above show() so they're in alphabetical order?

Updated

Edit: Missed comiting this, will be in follow-up

dylankcrwd · 2024-12-03T16:02:49Z

Code looks good to me, but I seem to find myself in a weird spot if I spam the open attribute toggle. I hit this by accident with hitting the spacebar initially and then the Drawer got stuck in this weird position.

Maybe not a real life scenario, but I'm wondering if this is a simple fix? It looks like the aside keeps both the closing open classes on the element no matter the open attribute once it gets in this state, so then it's stuck. What do you think?

I suspect this issue has always been there and that the reason is multiple transistionend events & listeners are fired close in time. The approach to animating this would need to change (e.g. web animation api). I'll look into it.

@ynotdraw I believe I have a solution to this issue. I'll create a follow-up pr.

ynotdraw · 2024-12-03T17:51:34Z

Sounds good, thanks @dylankcrwd !

dylankcrwd changed the title ~~feat(Drawer): add "open" attribute~~ feat(Drawer): add open attribute Dec 2, 2024

updates drawer, tests, and story

3bdfa91

dylankcrwd force-pushed the add-drawer-open-attribute branch from dea9ab5 to 3bdfa91 Compare December 2, 2024 20:39

add changeset

aa1633c

dylankcrwd marked this pull request as ready for review December 2, 2024 21:05

dylankcrwd requested review from clintcs, danwenzel and ynotdraw as code owners December 2, 2024 21:05

danwenzel approved these changes Dec 2, 2024

View reviewed changes

ynotdraw reviewed Dec 3, 2024

View reviewed changes

dylankcrwd added 2 commits December 3, 2024 09:01

pr feedback

4adf8b9

pr feedback, update multiple tests

74c65c2

clintcs approved these changes Dec 3, 2024

View reviewed changes

ynotdraw approved these changes Dec 3, 2024

View reviewed changes

dylankcrwd merged commit 37df083 into main Dec 3, 2024
7 checks passed

dylankcrwd deleted the add-drawer-open-attribute branch December 3, 2024 16:18

github-actions bot mentioned this pull request Dec 3, 2024

Release Preview #490

Merged

This was referenced Dec 4, 2024

fix(Drawer): improve animation responsiveness #502

Closed

fix(Drawer): use web animation api #507

Merged

	Adds an `open` attribute to Drawer.
	Drawer now supports an `open` attribute.

	expect(component.shadowRoot?.querySelector('aside[data-test-state="closed"]'))
	expect(component.shadowRoot?.querySelector('[data-test-state="closed"]'))


		await elementUpdated(component);

		expect(component.shadowRoot?.querySelector('aside[data-test-state="open"]'))

feat(Drawer): add open attribute #497

feat(Drawer): add open attribute #497

Uh oh!

Conversation

dylankcrwd commented Dec 2, 2024 • 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 Dec 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🦋 Changeset detected

Uh oh!

github-actions bot commented Dec 2, 2024

Preview URL

Uh oh!

ynotdraw left a comment • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylankcrwd commented Dec 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dylankcrwd Dec 3, 2024 • 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!

dylankcrwd commented Dec 3, 2024

Uh oh!

Uh oh!

ynotdraw commented Dec 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

feat(Drawer): add `open` attribute #497

feat(Drawer): add `open` attribute #497

dylankcrwd commented Dec 2, 2024 •

edited

Loading

changeset-bot bot commented Dec 2, 2024 •

edited

Loading

ynotdraw left a comment •

edited

Loading

dylankcrwd commented Dec 3, 2024 •

edited

Loading

dylankcrwd Dec 3, 2024 •

edited

Loading