Skip to content
This repository was archived by the owner on Nov 13, 2025. It is now read-only.

Prepare for the elements manifest #691

Merged
clintcs merged 8 commits into from
Feb 19, 2025
Merged

Prepare for the elements manifest #691

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
42f7cb6
Prepare for the elements manifest
clintcs Feb 6, 2025
84c4a50
Add Input `type` attribute changeset
clintcs Feb 18, 2025
6b40c11
Apply suggestions from code review
clintcs Feb 18, 2025
124ba18
Fix Menu with icons styling bug
clintcs Feb 18, 2025
35f103a
Make slot comment tag order consistent
clintcs Feb 18, 2025
a6cc288
Merge remote-tracking branch 'origin/staging' into prepare-for-cem
clintcs Feb 18, 2025
12c98d2
Merge remote-tracking branch 'origin/staging' into prepare-for-cem
clintcs Feb 19, 2025
0f3be97
Override Web Test Runner's Playwright
clintcs Feb 19, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/ten-monkeys-dress.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@crowdstrike/glide-core': patch
---

Input's `type` attribute is now reflected.
21 changes: 21 additions & 0 deletions .changeset/twenty-pandas-brake.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
---
'@crowdstrike/glide-core': minor
---

- Tab Panel no longer has an unused static `instanceCount` property.
- Toggle no longer has a `name` property. `name` only applies to form controls and was unused.
- Tree Item's `hasChildTreeItems` and `hasExpandIcon` properties and its `toggleExpand()` method have been marked private.

Additionally, some internal changes were made to facillitate generating documentation programmatically forced us remove a few exported types and rename some custom properties:

- Input no longer exports a `SUPPORTED_TYPES` interface.
- Toasts no longer exports a `Toast` interface.
- Tab Panel's custom properties have been renamed:

```diff
- --panel-padding-inline-end
+ --padding-inline-end

- --panel-padding-inline-start
+ --padding-inline-start
```
4 changes: 3 additions & 1 deletion .husky/pre-push
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

NODE_ENV=production pnpm typecheck && NODE_ENV=production pnpm test
NODE_ENV=production pnpm start
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll be shortly adding:

  • start:production:cem-analyze
  • start:developmenproduction:ts-morph

Those commands will fail if there's a difference between what they generate and what the developer has committed and is trying to push. So I thought it best to just run all of start:production so failures of those commands happen locally before push.

pnpm start will do more than just run those two commands. It'll also run start:production:components, start:production:storybook, andstart:production:stylesheets.

But I figured we should be running those, too, locally on push. Pushes will be slower. But a slower push is faster than realizing in CI after a push that something failed, eh?

NODE_ENV=production pnpm test
NODE_ENV=production pnpm typecheck
2 changes: 2 additions & 0 deletions .stylelintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ export default {
'stylelint-use-nesting',
'stylelint-use-logical',
'stylelint-order',
'./dist/stylelint/plugin',
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Back To A Monorepo

Last week, Tony and I were working through converting everything in /.storybook to TypeScript when it occurred to us that the TypeScript problems he was running into were because we're no longer a monorepo.

Ditching the monorepo simplified a lot for us. But we realized last week—especially with the addition of a custom Stylelint rule here and my upcoming additions of ./src/cem-analyzer-plugins and ./src/ts-morph directories to ./src—that at this point we're effectively in a multi-package repository but trying to pretend otherwise.

Our unfortunate conclusion was that we need to move back to a monorepo—and that this time it'll be worth the complication given everything we've added to this repository.

His TypeScript-related .storybook issues will go away. So will the awkwardness above: instead, the Stylelint plugin—like the ESLint one—will be its own package that will handle compilation and then export a compiled plugin that packages/components/.stylelintrc.js imports. Other issues related to the element analyzer will also go away.

],
rules: {
// https://github.com/w3c/csswg-drafts/issues/9496
Expand All @@ -17,5 +18,6 @@ export default {
'no-descending-specificity': null,
'order/properties-alphabetical-order': true,
'prettier/prettier': true,
'glide-core/no-unprefixed-private-custom-property': true,
},
};
8 changes: 4 additions & 4 deletions .vscode/settings.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,11 @@
{
"eslint.workingDirectories": [{ "mode": "auto" }],
"editor.formatOnSave": true,
"[javascript]": {
"editor.defaultFormatter": "esbenp.prettier-vscode"
},
"stylelint.validate": ["css", "postcss", "typescript"],
"[typescript]": {
"editor.defaultFormatter": "esbenp.prettier-vscode"
}
},
"editor.formatOnSave": true,
"eslint.workingDirectories": [{ "mode": "auto" }],
"stylelint.validate": ["css", "postcss", "typescript"]
}
69 changes: 26 additions & 43 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,44 +146,35 @@ Embrace encapsulation wherever you can.

#### Avoid styling `:host`

Styling `:host` exposes the styles to consumers—allowing internal styles to be overridden.
Due to that, we do not recommend styling `:host` in our components, but rather using CSS variables targeting the tag directly or using a class name.
Styling `:host` exposes styles to consumers—allowing internal styles to be overridden.
Style classes directly instead:

```css
/* ✅ -- GOOD */
/* Target the button tag directly */
button {
background-color: var(--button-background-color);
}

/* Or use a class name <button class="button" */
.button {
background-color: var(--button-background-color);
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best not to reference colors that don't exist.

display: flex;
}
```

```css
/* ❌ -- BAD */
/* Consumers can override via */
/* <cool-button style="background-color: red" which */
/* may not be your intention */
:host {
background-color: #4095bf;
display: flex;
}
```

If you have styles or style variables that apply to the whole component, consider styling a containing element instead.
If your component doesn't have a single containing element, you can add one:
If your component doesn't have a single containing element, simply add one:

```ts
// checkbox.ts
// component.ts
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more generic the better, so the guidance doesn't appear component-specific.

render() {
return html`<div class="component"></div>`
}
```

```ts
// checkbox.styles.ts
// component.styles.ts
import { css } from 'lit';

export default css`
Expand All @@ -194,28 +185,26 @@ export default css`

#### Avoid exposing `part`s

[`Part`s](https://developer.mozilla.org/en-US/docs/Web/CSS/::part) expose areas of your UI that consumers can target with CSS, which allows them to customize it to their needs.
Presently, we have no use case for exposing a `part`.
Instead, we should stick with exposing styles via CSS variables until the need arises.
[Parts](https://developer.mozilla.org/en-US/docs/Web/CSS/::part) expose tags within components to arbitrary styling by consumers.
We don't currently have a reason to allow arbitrary styling.
Until we do, use custom properties to allow only certain styles to be overridden.

```ts
// ✅ -- GOOD
@customElement('glide-core-example')
export default class GlideCoreExample extends LitElement {
static override styles = css`
.summary {
font-weight: var(--font-weight-bold);
:host {
--font-weight: 700;
}

.component {
font-weight: var(--font-weight);
}
`;

override render() {
return html`
<details>
<!-- We style the summary directly ourselves -->
<summary class="summary">Details</summary>
<div><slot></slot></div>
</details>
`;
return html`<button class="component">Button</button>`;
}
}
```
Expand All @@ -225,13 +214,7 @@ export default class GlideCoreExample extends LitElement {
@customElement('glide-core-example')
export default class GlideCoreExample extends LitElement {
override render() {
return html`
<details>
<!-- We do not want to expose a part -->
<summary part="summary">Details</summary>
<div><slot></slot></div>
</details>
`;
return html` <button part="component">Button</button>`;
}
}
```
Expand Down Expand Up @@ -347,24 +330,24 @@ If you need elements from a specific slot, use [assignedElements()](https://deve
#### Prefer using animations only when the user has no reduced motion preference

The [`prefers-reduced-motion`](https://developer.mozilla.org/en-US/docs/Web/CSS/@media/prefers-reduced-motion) media query is used to detect if a user has enabled a setting on their device to minimize inessential motion.
Our accessibility team recommends only enabling animations when the user doesn't prefer reduced motion.
Our accessibility team recommends only enabling animations when the user has that setting turned off.

```css
/* ✅ -- GOOD */
.animation {
background-color: purple;
transform: translateX(100%);
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A more realistic and simplified example that doesn't reference a color people shouldn't use.


@media (prefers-reduced-motion: no-preference) {
animation: pulse 1s linear infinite both;
transition: transform 1s;
}
}
```

```css
/* ❌ -- BAD */
.animation {
animation: pulse 1s linear infinite both;
background-color: purple;
transform: translateX(100%);
transition: transform 1s;
}
```

Expand Down Expand Up @@ -552,7 +535,7 @@ There are many ways to target the root element of a component in CSS; however, w
// ✅ -- GOOD
css`
.component {
background-color: red;
display: flex;
}
`;

Expand All @@ -565,7 +548,7 @@ render() {
// ❌ -- BAD
css`
div {
background-color: red;
display: flex;
}
`;

Expand Down Expand Up @@ -616,4 +599,4 @@ So it's best to always override and decorate (using `@property`) inherited prope
It helps clarify how different scripts are used in different contexts.
It also neatly abstracts away specific script names from CI configuration.

In general, think of `:development` scripts as either long-running (`--serve`, `--watch`) or mutative (`--fix`, `--write`) and `:production` scripts as neither of those things.
In general, think of `*:development:*` scripts as long-running (`--serve`, `--watch`) and mutative (`--fix`, `--write`) and `*:production:*` scripts as neither.
18 changes: 17 additions & 1 deletion eslint.config.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,11 @@ export default [
'@crowdstrike/glide-core/no-space-press': 'error',
'@crowdstrike/glide-core/prefer-shadow-root-mode': 'error',
'@crowdstrike/glide-core/prefixed-lit-element-class-declaration': 'error',
'@crowdstrike/glide-core/public-member-return-type': 'error',
'@crowdstrike/glide-core/public-getter-default-comment': 'error',
'@crowdstrike/glide-core/event-dispatch-from-this': 'error',
'@crowdstrike/glide-core/string-event-name': 'error',
'@crowdstrike/glide-core/slot-type-comment': 'error',

// Enabling this rule would force us to `await` any function that returns a promise.
// One example is a function that itself `await`s `updateComplete`. The rule is a bit
Expand Down Expand Up @@ -289,7 +294,6 @@ export default [
// and other times something else depending on what it's chained with.
...typescript.configs.disableTypeChecked,
},

{
files: ['src/**/*.test.ts', 'src/*.test.*.ts', 'src/*.*.test.*.ts'],
rules: {
Expand All @@ -307,6 +311,18 @@ export default [
'@crowdstrike/glide-core/no-skip-tests': 'error',
'@crowdstrike/glide-core/no-to-have-attribute': 'error',
'@crowdstrike/glide-core/prefer-to-be-true-or-false': 'error',
'@crowdstrike/glide-core/public-member-return-type': 'off',
'@crowdstrike/glide-core/public-getter-default-comment': 'off',
'@crowdstrike/glide-core/event-dispatch-from-this': 'off',
'@crowdstrike/glide-core/string-event-name': 'off',
'@crowdstrike/glide-core/slot-type-comment': 'off',
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All off for tests.

},
},
{
files: ['src/library/**'],
rules: {
'@crowdstrike/glide-core/public-member-return-type': 'off',
'@crowdstrike/glide-core/public-getter-default-comment': 'off',
},
},
];
76 changes: 45 additions & 31 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,64 +11,75 @@
},
"files": [
"dist",
"!dist/*.*.test.*.d.ts",
"!dist/*.*.test.*.js",
Comment on lines +14 to +15
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These two were missing. Also ran all these lines through VS Code's ascending sort. Did the same for some other lists.

Copy link
Collaborator

@ynotdraw ynotdraw Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch 👍 . I poke around at https://www.npmjs.com/package/@crowdstrike/glide-core?activeTab=code from time to time to see what's in there

Copy link
Collaborator Author

@clintcs clintcs Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep! Same.

"!dist/*.stories.d.ts",
"!dist/*.stories.js",
"!dist/*.test.*.d.ts",
"!dist/*.test.*.js",
"!dist/cem-analyzer-plugins",
"!dist/eslint",
"!dist/icons/storybook.*",
"!dist/storybook",
"!dist/*.test.js",
"!dist/*.test.*.d.ts",
"!dist/*.test.*.js",
"!dist/*.stories.js",
"!dist/*.stories.d.ts"
"!dist/stylelint",
"!dist/ts-morph"
],
"exports": {
"./*.js": {
"types": "./dist/*.d.ts",
"import": "./dist/*.js"
},
"./*.styles.js": null,
"./library/*": null,
"./icons/*": null,
"./styles/*": null,
"./label.js": null,
"./toasts.toast.js": null,
"./translations/*": null,
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

translations/* is new. Other than that just sorting.

"./tooltip.container.js": null,
"./toasts.toast.js": null,
"./styles/variables.css": "./dist/styles/variables.css",
"./styles/fonts.css": "./dist/styles/fonts.css",
"./styles/variables.css": "./dist/styles/variables.css"
"./styles/*": null,
"./library/*": null,
"./label.js": null,
"./icons/*": null,
"./*.styles.js": null
},
"pnpm": {
"overrides": {
"esbuild@0.24.2": "^0.25.0",
"koa@2.14.2": "^2.15.4"
"koa@2.14.2": "^2.15.4",
"playwright@1.49.1": "1.50.1"
}
},
"scripts": {
"start": "per-env",
"start:development": "storybook dev --config-dir .storybook --no-open --no-version-updates --port 6006",
"start:production": "rimraf ./dist && npm-run-all --parallel start:production:components start:production:storybook start:production:stylesheets --aggregate-output --print-label",
"start:production:components": "tsc --outDir ./dist && node ./terser.js",
"start:development": "npm-run-all --parallel --print-name start:development:*",
"start:development:storybook": "storybook dev --config-dir .storybook --no-open --no-version-updates --port 6006",
"start:production": "rimraf ./dist && npm-run-all --aggregate-output --print-label --parallel start:production:components start:production:storybook start:production:stylesheets",
"start:production:components": "tsc --noCheck --outDir ./dist && node ./terser.js",
"start:production:figma": "pnpm dt export-variables && pnpm dt build-tokens && pnpm dt build-styles",
"start:production:storybook": "storybook build --config-dir .storybook --disable-telemetry --output-dir ./dist/storybook",
"start:production:stylesheets": "node ./esbuild.js",
"format": "per-env",
"format:development": "prettier . --write && stylelint '**/*.styles.ts' --custom-syntax postcss-lit --fix",
"format:production": "prettier . --check && stylelint '**/*.styles.ts' --custom-syntax postcss-lit --no-color",
"format:development": "chokidar '**/*' --ignore 'dist/**' --ignore '.changeset/**' --ignore '.git/**' --ignore 'node_modules/**' --ignore 'pnpm-lock.yaml' --initial --silent --comand 'prettier --write --ignore-unknown {path} && stylelint {path}'",
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added Chokidar to watch files for a couple new commands I'll be adding next: start:development:cem-analyze and start:development:ts-morph.

Figured I'd add watching to our other *:development:* commands to match. If we don't want watching locally (I don't), we can just use the production commands.

But at least all our *:development:* commands are in alignment now that they're all long-running:

In general, think of *:development:* scripts as either long-running (--serve, --watch) or mutative (--fix, --write) and *:production:* scripts as neither.

"format:production": "prettier . --debug-check",
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for Prettier to change code in CI.

"lint": "per-env",
"lint:development": "npm-run-all --print-name lint:development:*",
"lint:development:eslint": "tsc --outDir ./dist && eslint . --fix",
"lint:development": "npm-run-all --parallel --print-name lint:development:*",
"lint:development:eslint": "chokidar ./eslint-config.js ./src/eslint/** ./src/*.ts ./src/*.*.ts --initial --silent --command 'tsc --noCheck --outDir ./dist && eslint {path} --fix'",
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running tsc when a file changes improves the lint-rule writing workflow slightly. Previously, you had a manually run tsc before the change would be picked up by your editor.

Also added --noCheck. We're not running tsc here to typecheck—only to compile JavaScript to ./dist. Same with lint:*:stylelint.

Copy link
Collaborator

@ynotdraw ynotdraw Feb 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Running tsc when a file changes improves the lint-rule writing workflow slightly. Previously, you had a manually run tsc before the change would be picked up by your editor.

A very welcome update. Thank you!

"lint:development:lit-analyzer": "lit-analyzer **/*.ts",
"lint:production": "npm-run-all --parallel --aggregate-output --print-label lint:production:*",
"lint:production:eslint": "tsc --outDir ./dist && eslint .",
"lint:development:stylelint": "chokidar ./.stylelintrc.js ./src/stylelint/** ./src/*.styles.ts ./src/*.*.styles.ts --initial --silent --command 'tsc --noCheck --outDir ./dist && stylelint {path} --custom-syntax postcss-lit --fix'",
"lint:production": "npm-run-all --aggregate-output --print-label --parallel lint:production:*",
"lint:production:eslint": "tsc --noCheck --outDir ./dist && eslint .",
"lint:production:lit-analyzer": "lit-analyzer **/*.ts",
"typecheck": "tsc --noEmit",
"lint:production:stylelint": "tsc --noCheck --outDir ./dist && stylelint '**/*.styles.ts' --custom-syntax postcss-lit --no-color",
"test": "per-env",
"test:development": "npm-run-all --parallel test:development:serve test:development:web-test-runner start:production:stylesheets",
"test:development:serve": "npx http-server dist/coverage/lcov-report --silent",
"test:development:web-test-runner": "web-test-runner --watch",
Copy link
Collaborator Author

@clintcs clintcs Feb 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed to test:development:components so it's clear what's under test. Renamed test:development:vitest for the same reason.

"test:development:vitest": "vitest --config ./vitest.config.js",
"test:development:vitest:comment": "Vitest is excluded from `test:development` because it muddies the console when running alongside Web Test Runner.",
"test:production": "npm-run-all --parallel test:production:* start:production:stylesheets --aggregate-output --print-label",
"test:production:vitest": "vitest run --config ./vitest.config.js",
"test:production:web-test-runner": "web-test-runner",
"test:development": "pnpm start:production:stylesheets && npm-run-all --parallel test:development:components test:development:serve-component-coverage",
"test:development:components": "web-test-runner --watch",
"test:development:lint-rules": "vitest --config ./vitest.config.js & chokidar './src/eslint/**' './src/stylelint/**' --ignore './src/eslint/rules/*.test.ts' './src/stylelint/*.test.ts' --initial --silent --command 'tsc --noCheck --outDir ./dist'",
"test:development:lint-rules:comment": "Excluded from `test:development` because Vitest muddies the console when running alongside Web Test Runner.",
"test:development:serve-component-coverage": "npx http-server dist/coverage/lcov-report --silent",
"test:production": "pnpm start:production:stylesheets && npm-run-all --aggregate-output --print-label --parallel test:production:*",
"test:production:lint-rules": "tsc --noCheck --outDir ./dist && vitest run --config ./vitest.config.js",
"test:production:components": "web-test-runner",
"typecheck": "per-env",
"typecheck:development": "tsc --outDir ./dist -w",
"typecheck:production": "tsc --outDir ./dist",
"postinstall": "pnpm dlx playwright@1.50.1 install --with-deps",
"prepare": "is-ci || husky install",
"release": "changeset publish"
Expand Down Expand Up @@ -112,6 +123,8 @@
"@web/test-runner-commands": "^0.9.0",
"@web/test-runner-playwright": "^0.11.0",
"chalk": "^5.3.0",
"chokidar-cli": "^3.0.0",
"comment-parser": "^1.4.1",
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A pretty widely used library. I'm using it to parse JSDoc-style comments in slots.

"esbuild": "^0.25.0",
"eslint": "^9.17.0",
"eslint-config-prettier": "^10.0.1",
Expand All @@ -129,6 +142,7 @@
"lit": "^3.2.1",
"lit-analyzer": "^2.0.3",
"minify-literals": "^1.0.10",
"node-html-parser": "^7.0.1",
Copy link
Collaborator Author

@clintcs clintcs Feb 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also widely used. Using it to parse markup in render() methods in that same lint rule. I'll also be using both libraries in an elements analyzer plugin.

"npm-run-all2": "^7.0.2",
"per-env": "^1.0.2",
"postcss": "^8.4.49",
Expand Down
Loading