Various documentation improvements #385

clintcs · 2024-09-17T20:44:48Z

🚀 Description

Added click() and focus() controls to stories. For context: Rework Split Button #361 (comment).
Reordered argTypes to match the order of args.
Emptied the slot="description" argument in form control stories after some discussion. See CONTRIBUTING.md.
Add missing controls to Tab's story and renamed the story to Tab Group.
Removed unnecessary instances of control: { type: 'boolean' } and control: { type: 'text' }, both of which are inferred by Storybook.
Replaced render: () => {} with render() {} for consistency.
Modified various stories whose controls accept markup to use unsafeHTML. This lets Storybook users test out their markup.

Allowing arbitrary markup is safe because Storybook doesn't push those controls with the URL. And, even if it did, we're not setting cookies or otherwise storing sensitive data vulnerable to an XSS.
Added a focus() method to Accordion and a click() method to Icon Button.
Removed story and component descriptions.

Wasn't sure about this. Talked to @ynotdraw about it offline. Many of our descriptions are superfluous (Menu's "A basic menu") and the rest are either implied or include information already present in the controls table.
Swapped out styles in stories for top-level decorators property where possible.
Added a Mutation Observer to Dropdown's story to account for Dropdown closing when it loses focus or something outside of it is clicked.

📋 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

Spotcheck every story.
Verify Accordion's focus() method.

📸 Images/Videos of Functionality

N/A

changeset-bot · 2024-09-17T20:44:52Z

🦋 Changeset detected

Latest commit: 30cdced

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

clintcs · 2024-09-17T21:09:47Z

.changeset/quick-dogs-rest.md

+'@crowdstrike/glide-core': minor
+---
+
+Menu no longer offers a `focus()` method, which focused its target. Simply call `focus()` on your target directly.


No need for a middleman, I figured.

Have we checked consumers to see if they're using this?

I haven't because it wouldn't affect the outcome for me. Whether they're using it or not, it's superfluous for Menu to offer a method already offered by their target button.

If it were in use, would that change the calculus for you?

Sorry, I should have said more: I'm cool with this change either way, we'd just need to be sure to communicate that this is a possible breaking change when it comes time to release. But we're good about doing that anyway, so I probably don't need to say it :)

Haha. Got it. Yeah, that's implied (though not obvious) by minor for us. But, yeah, I trust that @ynotdraw will spread the good word.

🫡 you bet!

clintcs · 2024-09-17T21:11:14Z

src/tabs.stories.ts


 export default meta;

-export const Tabs: StoryObj = {


Do we need an overflow story? Or can we just show overflow in the primary story?

I'd vote for a separate overflow story. Because it seems to me the most common case is no overflow.

I can dig it. I'll revert this.

✅ 30cdced

github-actions · 2024-09-17T21:14:18Z

Preview URL

https://glide-core.crowdstrike-ux.workers.dev/moar-documentation-improvements

src/button.stories.ts

clintcs · 2024-09-19T19:01:47Z

src/checkbox-group.stories.ts

    'slot="tooltip"': {
-      control: { type: 'text' },
      table: {
-        type: { summary: 'HTMLKBDElement | string' },


Slotting a string into a named slot is actually impossible. Derp.

My first thought was to change this to HTMLKBDElement | Element. But Element includes HTMLKBDElement. So I went with Element and thought I'd let the control, which is prominent and easy to read, speak for the <kbd> part.

Similar thinking for the 'slot="description"' changes above and throughout.

clintcs · 2024-09-19T19:12:07Z

src/button-group.stories.ts

-      import '@crowdstrike/glide-core/button-group.js';
-      import '@crowdstrike/glide-core/button-group.button.js';
-    </script>
+  render(arguments_) {


render: () => {} → render() {}

Probably lintable, eh?

Almost certainly. There's a lot I'd like us to lint in stories. The order of top-level is meta properties is another.

Let me get our lint rule guy on the horn 😆. CC: @ynotdraw.

☎️ you rang!?!??!

(I'll add this to my list)

clintcs · 2024-09-19T19:26:16Z

src/button-group.stories.ts

+        <glide-core-button-group-button label="Two">
+          <glide-core-example-icon
+            slot="prefix"
+            name="pencil"


Better looking, eh?

Before

After

clintcs · 2024-09-19T19:27:40Z

src/accordion.stories.ts

      },
      type: { name: 'string', required: true },
    },
-    open: {


Most other argTypes changes below and throughout are just from reordering to match the order of args.

clintcs · 2024-09-19T19:34:23Z

src/split-button-container.stories.ts

-      <glide-core-menu-link label="Three" url="/"></glide-core-menu-link>
-    </glide-core-split-button-container>
-  `,
+        <glide-core-menu-button label="One"></glide-core-menu-button>


Two buttons and one link, same as Split Button Container. Most users will probably click the first or second option, which means they won't be suddenly navigated like they would with a link. But a link is still available if they want to click it.

clintcs · 2024-09-19T20:20:06Z

CONTRIBUTING.md

 A story showing the use of an optional slot that requires specific markup is one example.
 A story showing an error state triggered by a form submission is another.

+### Only set required attributes in stories


One more of these and we might think about putting Storybook best practices into their own section.

clintcs · 2024-09-19T20:20:55Z

CONTRIBUTING.md

+### Only set required attributes in stories
+
+While it's useful to present components in their full form, we think there's more value in providing minimal code examples so consumers don't copy more code than they need.
+Only giving required attributes a value also gives us a simple rule to follow when writing stories.


I wanted an example to accompany this but couldn't come up with a clear and simple one.

danwenzel

🎉 all around

src/button.stories.ts

danwenzel · 2024-09-20T16:29:38Z

src/icon-button.test.focus.ts

+  height="16"
+  stroke="currentColor"
+  fill="none"
+  stroke-linecap="round"


nit: Can we ditch any of attributes that aren't necessary?

This is copy pasta from another Icon Button test. But 100% we can. Is this the only one that's unnecessary?

I'm guessing you could probably remove most of them :). I say if the test passes, it passes 🤷

Ah. I see where you're coming from. I was thinking you still wanted the icon to be presentable.

Strictly speaking, it doesn't even need to be an icon. Other components just throw a <div> Icon </div> in there and call it a day.

Icon Button's tests need to be cleaned up in other ways. So I can take both the cleanup and this as a followup if want to go the <div> route.

danwenzel · 2024-09-20T16:32:47Z

src/tabs.stories.ts


 export default meta;

-export const Tabs: StoryObj = {


I'd vote for a separate overflow story. Because it seems to me the most common case is no overflow.

clintcs force-pushed the moar-documentation-improvements branch 2 times, most recently from 0f4c328 to 19deecb Compare September 17, 2024 21:09

clintcs commented Sep 17, 2024

View reviewed changes

clintcs force-pushed the moar-documentation-improvements branch 2 times, most recently from 2aa29ab to 44d7c85 Compare September 19, 2024 17:24