feat(split button): adds split button #207

dylankcrwd · 2024-06-20T15:47:00Z

Adds glide-core-split-container, glide-core-split-button, and glide-core-split-link.

Storybook: https://glide-core.crowdstrike-ux.workers.dev/add-split-button?path=/docs/split-button--overview

changeset-bot · 2024-06-20T15:47:03Z

🦋 Changeset detected

Latest commit: 9e906ca

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-06-20T15:50:52Z

Preview URL

https://glide-core.crowdstrike-ux.workers.dev/add-split-button

ynotdraw

Looks good @dylankcrwd ! A few questions/comments.

One other thing comes to mind, I imagine design already approved, but with the small variant should the dropdown arrow also change sizing? Without it, the chevron down is larger than the text. Curious what design thinks.

.changeset/perfect-yaks-cheer.md

packages/components/src/split-button.stories.ts

ynotdraw · 2024-06-27T12:28:52Z

packages/components/src/split-button.styles.ts

+      border-radius: var(--glide-core-spacing-sm) 0 0
+        var(--glide-core-spacing-sm);


Bummer we don't have a --glide-core-border-radius that matches this. I'm a bit hesitant on us using spacing values for border radii. I'd maybe prefer just hardcoding these values instead of using spacing-sm here, because if that value changes for spacing, it would adjust the radii here. What do y'all think?

Updated.

Since this was copied from glide-core-button, should that change as well?

I would probably say so, yeah. I think it's safer for us. Thanks for looking at glide-core-button too!

Updated glide-core-button

packages/components/src/split-button.styles.ts

packages/components/src/split-button.test.basics.ts

ynotdraw · 2024-06-27T12:53:13Z

packages/components/src/split-container.ts

+  }
+
+  // Can't disable `glide-core-menu`, so capture the event before it arrives.
+  // Disabling the trigger button doesn't resolve the issue.


Is this a bug potentially with Menu @clintcs ? I guess maybe not, since the menu doesn't necessarily know if the target it is referencing is disabled or not. And maybe determining that from within menu would be tricky?

I could see it being Menu's responsibility to check if the target is a child of an element with a disabled property and return early if so.

Shall we file a task for this or what do you think? Is it a quick update that we could roll into this PR? Then we could remove some code here and get a nice bonus in Menu with this PR?

I would prefer to make it a separate task I could do in the next sprint, otherwise I fear this card won't get done in time and since I'm OOO for much of next week, the branch will go quite stale.

I leave it to you two. The work is done on my side. I can push a commit or open a separate PR.

@clintcs If you have a commit with the desired change could you please push it to this branch? Thanks!
I can then remove the click interception logic and related test.

Sweet. I'll push the commit shortly.

@dylankcrwd: Go ahead and merge as is. Filling test gaps in Menu and accounting for the case where disabled is removed from the target have resulted in a big diff.

Thank you @clintcs

@dylankcrwd: See the comment on my Menu PR for context. I rebased your branch onto main to bring in my changes and then pushed a commit removing your workaround for Menu. You should be good.

In rebasing I amended this branch's history. So locally you'll need to fetch and then git reset --hard origin/add-split-button.

packages/components/src/split-link.test.basics.ts

packages/components/src/split-link.test.events.ts

ynotdraw · 2024-06-27T13:00:34Z

packages/components/src/split-link.ts

+      href=${ifDefined(this.url)}
+      class=${classMap({
+        component: true,
+        disabled: this.disabled,


For a11y reasons, when a link should be disabled, we actually render a <span> with some attributes instead. That way the component appears visually disabled, but then we don't have an underlying <a that we have to intercept click handlers for and it also announces to screenreaders as one would expect.

This happened in one of our other private repos (you know the one!) with main-navigation-link.ts if you take a look there. We should probably follow the same pattern here as that was blessed by Noah from an a11y standpoint.

Will look into this

Updated.

To be consistent with a disabled button, the span is not tabbable when disabled, unlike main-navigation-link.ts. Let me know if this is what you're looking for.

dylankcrwd · 2024-06-27T15:18:02Z

Looks good @dylankcrwd ! A few questions/comments.

One other thing comes to mind, I imagine design already approved, but with the small variant should the dropdown arrow also change sizing? Without it, the chevron down is larger than the text. Curious what design thinks.

This looks to be consistent with glide-core-button. Maybe this should be a different card?

The split button has been approved, however there is nothing that I've seen in the figma for the small size and was something I added to be consistent with the button.

ynotdraw · 2024-06-27T19:06:57Z

The split button has been approved, however there is nothing that I've seen in the figma for the small size and was something I added to be consistent with the button.

Oh. If this is the case, I don't think we want to add sizing for this component. I'd double check with Mary, Joe, or Zheng. We only want to add support for things we know design explicitly calls for. So even though Button supports a size, there may not be needs for a small SplitButton, so we shouldn't add it.

dylankcrwd · 2024-06-27T19:26:55Z

The split button has been approved, however there is nothing that I've seen in the figma for the small size and was something I added to be consistent with the button.

Oh. If this is the case, I don't think we want to add sizing for this component. I'd double check with Mary, Joe, or Zheng. We only want to add support for things we know design explicitly calls for. So even though Button supports a size, there may not be needs for a small SplitButton, so we shouldn't add it.

I asked the question of Design & ~~awaiting their reply~~ they asked to keep it.

ynotdraw · 2024-06-28T18:31:04Z

packages/components/src/split-button.styles.ts

+        border-color: transparent;
+        color: var(--glide-core-text-selected);
+
+        &.disabled {


Could be good to add a comment on why this uses .disabled versus :disabled (because of the shared usage of buttons and links) for future travelers.

The comment is just above on line 37, but I can move it down.

ynotdraw · 2024-06-28T18:36:22Z

packages/components/src/split-container.test.interactions.ts

+    ?.dispatchEvent(event);
+
+  await elementUpdated(component);
+


Since the test reads "remains closed when clicked", should we verify once again that the menu is in fact closed and not open? Similar to what you have in the next test at the bottom.

Suggested change

expect(

component.shadowRoot?.querySelector('glide-core-menu'),

).to.not.have.attribute('open');

ynotdraw · 2024-06-28T18:38:26Z

packages/components/src/split-link.test.basics.ts

+  expect(link?.classList.contains('secondary')).to.be.true;
+});
+
+it('becomse a span when the "disabled" attribute exists', async () => {


Typo:

Suggested change

it('becomse a span when the "disabled" attribute exists', async () => {

it('becomes a span when the "disabled" attribute exists', async () => {

ynotdraw · 2024-06-28T18:38:54Z

packages/components/src/split-link.test.basics.ts

+  );
+});
+
+it('throws an error when there is something other than text is in the default slot', async () => {


A small typo:

Suggested change

it('throws an error when there is something other than text is in the default slot', async () => {

it('throws an error when there is something other than text in the default slot', async () => {

dylankcrwd force-pushed the add-split-button branch 2 times, most recently from f764353 to e33d283 Compare June 26, 2024 20:24

dylankcrwd marked this pull request as ready for review June 26, 2024 20:49

dylankcrwd requested review from clintcs, danwenzel and ynotdraw as code owners June 26, 2024 20:49

ynotdraw reviewed Jun 27, 2024

View reviewed changes

dylankcrwd requested a review from ynotdraw June 28, 2024 16:42

ynotdraw reviewed Jun 28, 2024

View reviewed changes

ynotdraw approved these changes Jun 28, 2024

View reviewed changes

dylankcrwd requested a review from ynotdraw June 28, 2024 19:16

clintcs mentioned this pull request Jul 1, 2024

Menu improvements #223

Merged

dylankcrwd and others added 9 commits July 2, 2024 16:26

add split button

b6eca12

add changeset

3ac1f9b

pr feedback

4d06186

pr feedback

1841fb2

restore box-sizing on small and large styling

0c2fd3c

update split link to use a link when disabled

b61087b

pr feedback

48f6059

pr feedback

05c339e

Remove Menu workaround

9e906ca

clintcs force-pushed the add-split-button branch from 0b7a551 to 9e906ca Compare July 2, 2024 21:26

ynotdraw approved these changes Jul 2, 2024

View reviewed changes

dylankcrwd merged commit b5d9180 into main Jul 8, 2024
7 checks passed

dylankcrwd deleted the add-split-button branch July 8, 2024 12:43

github-actions bot mentioned this pull request Jul 8, 2024

Release Preview #213

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(split button): adds split button #207

feat(split button): adds split button #207

dylankcrwd commented Jun 20, 2024 •

edited

Loading

changeset-bot bot commented Jun 20, 2024 •

edited

Loading

github-actions bot commented Jun 20, 2024

ynotdraw left a comment

ynotdraw Jun 27, 2024

dylankcrwd Jun 27, 2024

ynotdraw Jun 27, 2024

dylankcrwd Jun 27, 2024 •

edited

Loading

ynotdraw Jun 27, 2024

clintcs Jun 27, 2024

ynotdraw Jun 27, 2024

dylankcrwd Jun 27, 2024

clintcs Jun 27, 2024

dylankcrwd Jun 28, 2024 •

edited

Loading

clintcs Jun 28, 2024

clintcs Jun 28, 2024

dylankcrwd Jun 28, 2024

clintcs Jul 2, 2024

ynotdraw Jun 27, 2024

dylankcrwd Jun 27, 2024

dylankcrwd Jun 27, 2024

dylankcrwd commented Jun 27, 2024 •

edited

Loading

ynotdraw commented Jun 27, 2024

dylankcrwd commented Jun 27, 2024 •

edited

Loading

ynotdraw Jun 28, 2024

dylankcrwd Jun 28, 2024

dylankcrwd Jun 28, 2024

ynotdraw Jun 28, 2024

dylankcrwd Jun 28, 2024

ynotdraw Jun 28, 2024

dylankcrwd Jun 28, 2024

ynotdraw Jun 28, 2024

dylankcrwd Jun 28, 2024

		border-radius: var(--glide-core-spacing-sm) 0 0
		var(--glide-core-spacing-sm);

+  expect(
+      component.shadowRoot?.querySelector('glide-core-menu'),
+    ).to.not.have.attribute('open');

	it('becomse a span when the "disabled" attribute exists', async () => {
	it('becomes a span when the "disabled" attribute exists', async () => {

	it('throws an error when there is something other than text is in the default slot', async () => {
	it('throws an error when there is something other than text in the default slot', async () => {

feat(split button): adds split button #207

feat(split button): adds split button #207

Conversation

dylankcrwd commented Jun 20, 2024 • edited Loading

changeset-bot bot commented Jun 20, 2024 • edited Loading

🦋 Changeset detected

github-actions bot commented Jun 20, 2024

Preview URL

ynotdraw left a 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

dylankcrwd Jun 27, 2024 • edited Loading

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

dylankcrwd Jun 28, 2024 • edited Loading

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

Choose a reason for hiding this comment

dylankcrwd commented Jun 27, 2024 • edited Loading

ynotdraw commented Jun 27, 2024

dylankcrwd commented Jun 27, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dylankcrwd commented Jun 20, 2024 •

edited

Loading

changeset-bot bot commented Jun 20, 2024 •

edited

Loading

dylankcrwd Jun 27, 2024 •

edited

Loading

dylankcrwd Jun 28, 2024 •

edited

Loading

dylankcrwd commented Jun 27, 2024 •

edited

Loading

dylankcrwd commented Jun 27, 2024 •

edited

Loading