AnglePickerColor: migrate Emotion to style.module#73786
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
066b192 to
f76881d
Compare
| <div | ||
| ref={ angleCircleRef } | ||
| onMouseDown={ startDrag } | ||
| className="components-angle-picker-control__angle-circle" |
There was a problem hiding this comment.
All the components-angle-picker-control-* class names are unused, so I removed them.
There was a problem hiding this comment.
We've been keeping all these for back compat, unfortunately 😅
For example:
gutenberg/packages/components/src/date-time/time/index.tsx
Lines 141 to 143 in b9e9788
There was a problem hiding this comment.
OK I brought the class names back 🙂 When they are together with the module classes in one clsx call, they suddently start looking very suspicious and redundant 🙂
|
Size Change: +566 B (+0.02%) Total Size: 2.57 MB
ℹ️ View Unchanged
|
| const classes = clsx( 'components-angle-picker-control', className ); | ||
|
|
||
| const unitText = <UnitText>°</UnitText>; | ||
| const unitText = <Text className={ styles[ 'unit-text' ] }>°</Text>; |
There was a problem hiding this comment.
This is interesting, my Storybook displays this correctly. The margin and color styles are potentially conflicting, but in my case the .unit-text one wins:
Both class names have the same specificity, so it all depends on insertion order. Either Emotion inserts its PolymorphicDiv-Text styles first, or the style.module.scss inserts its styles first. Both are dynamically added <style> elements.
There was a problem hiding this comment.
Is the something fixable? Don’t like the idea of relying on random order.
There was a problem hiding this comment.
I think we could remove the Text component and the custom CSS styles, and just use InputControlSuffixWrapper for the padding. It doesn't make sense that the degree symbol is blue, it's not interactive.
There was a problem hiding this comment.
Is the something fixable? Don’t like the idea of relying on random order.
When both styles used Emotion, like
const Text = styled.div`color: blue`;
const UnitText = styled(Text)`color: red`;
then Emotion guaranteed that the red color always wins. But now when the red style is not Emotion, we don't have that guarantee any more and have to implement it ourselves somehow. Let me see what we can do.
There was a problem hiding this comment.
I think we could remove the
Textcomponent and the custom CSS styles, and just useInputControlSuffixWrapperfor the padding
@mirka Would you mind pushing a commit that implements this? You probably know much better than me what exactly needs to be done.
We have at least one learning for further migration: if one styled component depends on another styled component, we should migrate the dependency first. That keeps the order of stylesheets.
style.module.css styles are inserted early, immediately when the using module is initialized. And Emotion styles are inserted lazily, at the moment when the component that uses them is mounted.
There was a problem hiding this comment.
Done in 4203b46 ✅
We have at least one learning for further migration
And also to remove this instruction 😄
- if the style element already has an additional class name, like
components-something, verify if that class name is really used in CSS styles. Remove it if it isn't.
0892056 to
4203b46
Compare
| : [ null, unitText ]; | ||
| const prefixOrSuffixProp = isRTL() | ||
| ? { prefix: <InputControlPrefixWrapper>°</InputControlPrefixWrapper> } | ||
| : { suffix: <InputControlSuffixWrapper>°</InputControlSuffixWrapper> }; |
There was a problem hiding this comment.
By the way, while we are here, is it really correct to use either prefix or suffix depending on RTL? I'd expect that "prefix" and "suffix" are semantic terms, something like CSS logical properties (*-start, *-end), and that the NumberControl would take care of displaying them in the right order.
There was a problem hiding this comment.
Thanks for reminding me, there should be a code comment here (169ecc3). I was actually confused too and looked up the history to make sure.
The prefix and suffix slots reposition themselves "semantically" by default, but we're intentionally overriding that behavior here.
There was a problem hiding this comment.
Thanks for updating the angle input. I verified that in an RTL locale the "degrees" unit is diplayed on the right side, just like in LTR. While units like "px" are moved to the left.
12a907a to
4da6230
Compare
Move the (already small) Draggable stylesheet to a CSS module so its rules travel via `@wordpress/style-runtime` (and therefore into any iframe wrapped in `<StyleProvider>` — e.g. the block-editor canvas) without needing the package-level `build-style/style.css` bundle. Drops the `@use` line from `packages/components/src/style.scss`, following the same shape as the `AlignmentMatrixControl` (#73714/#73757) and `AnglePickerControl` (#73786) migrations. The CSS-module class names are standard (hashed). The legacy `components-draggable__*` / `is-dragging-components-draggable` class names are kept by adding them alongside the hashed ones in the JS `classList.add(...)` calls, since several other Gutenberg packages reference them in their own stylesheets (block-editor's `list-view`, `block-tools`, `block-library`'s `navigation` editor, `edit-widgets`' `widget-area` editor) and block-editor runtime JS reads `is-dragging-components-draggable` off `document.body`. Dropping those names would silently break those consumers. Per mirka's review on PR #78183 (CSS-module option for the iframe story); the corresponding Storybook simplification follows in a separate commit.
Move the (already small) Draggable stylesheet to a CSS module so its rules travel via `@wordpress/style-runtime` (and therefore into any iframe wrapped in `<StyleProvider>` — e.g. the block-editor canvas) without needing the package-level `build-style/style.css` bundle. Drops the `@use` line from `packages/components/src/style.scss`, following the same shape as the `AlignmentMatrixControl` (#73714/#73757) and `AnglePickerControl` (#73786) migrations. The CSS-module class names are standard (hashed). The legacy `components-draggable__*` / `is-dragging-components-draggable` class names are kept by adding them alongside the hashed ones in the JS `classList.add(...)` calls, since several other Gutenberg packages reference them in their own stylesheets (block-editor's `list-view`, `block-tools`, `block-library`'s `navigation` editor, `edit-widgets`' `widget-area` editor) and block-editor runtime JS reads `is-dragging-components-draggable` off `document.body`. Dropping those names would silently break those consumers. Per mirka's review on PR #78183 (CSS-module option for the iframe story); the corresponding Storybook simplification follows in a separate commit.
Move the (already small) Draggable stylesheet to a CSS module so its rules travel via `@wordpress/style-runtime` (and therefore into any iframe wrapped in `<StyleProvider>` — e.g. the block-editor canvas) without needing the package-level `build-style/style.css` bundle. Drops the `@use` line from `packages/components/src/style.scss`, following the same shape as the `AlignmentMatrixControl` (#73714/#73757) and `AnglePickerControl` (#73786) migrations. The CSS-module class names are standard (hashed). The legacy `components-draggable__*` / `is-dragging-components-draggable` class names are kept by adding them alongside the hashed ones in the JS `classList.add(...)` calls, since several other Gutenberg packages reference them in their own stylesheets (block-editor's `list-view`, `block-tools`, `block-library`'s `navigation` editor, `edit-widgets`' `widget-area` editor) and block-editor runtime JS reads `is-dragging-components-draggable` off `document.body`. Dropping those names would silently break those consumers. Per mirka's review on PR #78183 (CSS-module option for the iframe story); the corresponding Storybook simplification follows in a separate commit.
Move the (already small) Draggable stylesheet to a CSS module so its rules travel via `@wordpress/style-runtime` (and therefore into any iframe wrapped in `<StyleProvider>` — e.g. the block-editor canvas) without needing the package-level `build-style/style.css` bundle. Drops the `@use` line from `packages/components/src/style.scss`, following the same shape as the `AlignmentMatrixControl` (#73714/#73757) and `AnglePickerControl` (#73786) migrations. The CSS-module class names are standard (hashed). The legacy `components-draggable__*` / `is-dragging-components-draggable` class names are kept by adding them alongside the hashed ones in the JS `classList.add(...)` calls, since several other Gutenberg packages reference them in their own stylesheets (block-editor's `list-view`, `block-tools`, `block-library`'s `navigation` editor, `edit-widgets`' `widget-area` editor) and block-editor runtime JS reads `is-dragging-components-draggable` off `document.body`. Dropping those names would silently break those consumers. Per mirka's review on PR #78183 (CSS-module option for the iframe story); the corresponding Storybook simplification follows in a separate commit.
Move the (already small) Draggable stylesheet to a CSS module so its rules travel via `@wordpress/style-runtime` (and therefore into any iframe wrapped in `<StyleProvider>` — e.g. the block-editor canvas) without needing the package-level `build-style/style.css` bundle. Drops the `@use` line from `packages/components/src/style.scss`, following the same shape as the `AlignmentMatrixControl` (#73714/#73757) and `AnglePickerControl` (#73786) migrations. The CSS-module class names are standard (hashed). The legacy `components-draggable__*` / `is-dragging-components-draggable` class names are kept by adding them alongside the hashed ones in the JS `classList.add(...)` calls, since several other Gutenberg packages reference them in their own stylesheets (block-editor's `list-view`, `block-tools`, `block-library`'s `navigation` editor, `edit-widgets`' `widget-area` editor) and block-editor runtime JS reads `is-dragging-components-draggable` off `document.body`. Dropping those names would silently break those consumers. Per mirka's review on PR #78183 (CSS-module option for the iframe story); the corresponding Storybook simplification follows in a separate commit.
Move the (already small) Draggable stylesheet to a CSS module so its rules travel via `@wordpress/style-runtime` (and therefore into any iframe wrapped in `<StyleProvider>` — e.g. the block-editor canvas) without needing the package-level `build-style/style.css` bundle. Drops the `@use` line from `packages/components/src/style.scss`, following the same shape as the `AlignmentMatrixControl` (#73714/#73757) and `AnglePickerControl` (#73786) migrations. The CSS-module class names are standard (hashed). The legacy `components-draggable__*` / `is-dragging-components-draggable` class names are kept by adding them alongside the hashed ones in the JS `classList.add(...)` calls, since several other Gutenberg packages reference them in their own stylesheets (block-editor's `list-view`, `block-tools`, `block-library`'s `navigation` editor, `edit-widgets`' `widget-area` editor) and block-editor runtime JS reads `is-dragging-components-draggable` off `document.body`. Dropping those names would silently break those consumers. Per mirka's review on PR #78183 (CSS-module option for the iframe story); the corresponding Storybook simplification follows in a separate commit.
Move the (already small) Draggable stylesheet to a CSS module so its rules travel via `@wordpress/style-runtime` (and therefore into any iframe wrapped in `<StyleProvider>` — e.g. the block-editor canvas) without needing the package-level `build-style/style.css` bundle. Drops the `@use` line from `packages/components/src/style.scss`, following the same shape as the `AlignmentMatrixControl` (#73714/#73757) and `AnglePickerControl` (#73786) migrations. The CSS-module class names are standard (hashed). The legacy `components-draggable__*` / `is-dragging-components-draggable` class names are kept by adding them alongside the hashed ones in the JS `classList.add(...)` calls, since several other Gutenberg packages reference them in their own stylesheets (block-editor's `list-view`, `block-tools`, `block-library`'s `navigation` editor, `edit-widgets`' `widget-area` editor) and block-editor runtime JS reads `is-dragging-components-draggable` off `document.body`. Dropping those names would silently break those consumers. Per mirka's review on PR #78183 (CSS-module option for the iframe story); the corresponding Storybook simplification follows in a separate commit.
Move the (already small) Draggable stylesheet to a CSS module so its rules travel via `@wordpress/style-runtime` (and therefore into any iframe wrapped in `<StyleProvider>` — e.g. the block-editor canvas) without needing the package-level `build-style/style.css` bundle. Drops the `@use` line from `packages/components/src/style.scss`, following the same shape as the `AlignmentMatrixControl` (#73714/#73757) and `AnglePickerControl` (#73786) migrations. The CSS-module class names are standard (hashed). The legacy `components-draggable__*` / `is-dragging-components-draggable` class names are kept by adding them alongside the hashed ones in the JS `classList.add(...)` calls, since several other Gutenberg packages reference them in their own stylesheets (block-editor's `list-view`, `block-tools`, `block-library`'s `navigation` editor, `edit-widgets`' `widget-area` editor) and block-editor runtime JS reads `is-dragging-components-draggable` off `document.body`. Dropping those names would silently break those consumers. Per mirka's review on PR #78183 (CSS-module option for the iframe story); the corresponding Storybook simplification follows in a separate commit.
…ot (#78183) * Draggable: Migrate clone wrapper to wp compat overlay slot Replace the legacy body-level / element-wrapper placement and its `z-index: 1000000000` with a portal-style migration onto the `@wordpress/ui` compat overlay slot (#77851). When the slot is available, the drag clone joins the slot's body-level stacking context across all three placement modes, so an active drag automatically shares stacking with any `@wordpress/ui` overlay opened mid-drag without needing per-version z-index races. Auto-enabled in WordPress environments via the slot helper's `window.wp.components` auto-detect; standalone hosts that bundle `@wordpress/components` directly fall back to the previous placement until they call `useEnableWpCompatOverlaySlot()`. `@wordpress/components` imports `getWpCompatOverlaySlot()` directly from `@wordpress/ui`'s public exports (also promoted from internal in this change). The `@wordpress/components` dep on `@wordpress/ui` is transitional, scoped to the legacy-overlay migration. Cross-document drags (e.g. dragging an element inside an iframe while the slot is in the parent document) fall back to the previous placement so the clone's viewport-relative geometry stays in a single coordinate space. The default placement mode (`appendToOwnerDocument: false`, no `dragComponent`) previously appended the clone to the dragged element's parent. In WP environments where the slot is now in effect, the clone instead lives in the slot — a body-level location. In-repo ripgrep finds no CSS or event-delegation scoping anchored to the clone's previous in-flow parent; external consumers that relied on that ancestry must either not opt into the slot or migrate their scoping. * Draggable: Storybook: render docs-page stories in iframes The drag clone uses `position: fixed`, which Storybook's docs-page wrappers break because they apply `transform`s that establish new containing blocks. As a result the clone resolves against those wrappers instead of the viewport and lands in the wrong place on the autodocs page. Render each Draggable story in its own iframe on the autodocs page via `parameters.docs.story.inline: false`, which restores viewport-relative positioning for the clone. * Draggable: Storybook: polish cross-document fallback playground story Three small follow-ups on the iframe regression story: - Inject the Draggable SCSS into the iframe via Vite's `?inline` import (same pattern `WithGlobalCSS` uses with `global-basic.scss?inline`) instead of duplicating rule bodies in `srcDoc`. Single source of truth; future SCSS edits flow through automatically. - Guard the style injection on `iframeDoc?.head` so the brief about:blank → srcDoc transition doesn't throw on the initial `useEffect` pass. - Align the slot-presence display with the public `getWpCompatOverlaySlot()` API: it now returns `undefined` rather than `null` when no slot is registered. * CHANGELOG: Restore entries dropped during rebase Three Unreleased entries were inadvertently removed when the Draggable migration commit was rebased onto trunk: - @wordpress/components Internal: `Modal`, `Menu`, `DropdownMenu` motion-token adoption (#76097). - @wordpress/components Internal: `Popover` close-button z-index cleanup (#78180). - @wordpress/ui Bug Fixes: `Text` CSS-defense values for paragraph and heading variants (#78172). Restore them under their original headings. * Storybook: Fix popover-with-slotfill cross-iframe collision boundary `Popover.Popup` stopped accepting `collisionBoundary` directly when #78168 introduced the `Popover.Positioner` slot subcomponent. The prop is now silently ignored on `Popup`, so the cross-iframe story's collision avoidance regressed after the rebase onto trunk. Route the boundary through `Popover.Positioner` (matching the modern `Popover.Popup`'s `positioner` slot pattern) so the popup honors the iframe's clipping edge again. This file is `.jsx` so the type system didn't catch the silent prop-drop. * Draggable: Storybook: refresh AppendElementToOwnerDocument JSDoc The story's JSDoc still described the legacy "escape ancestor stacking context" rationale, which now contradicts the updated `appendToOwnerDocument` JSDoc in `types.ts` for hosts that opt into the `@wordpress/ui` compat overlay slot — where the clone always lives in the body-level slot and the prop is a no-op. Update the story's docblock to mirror the type-level guidance and call out the cross-document fallback exception. * Draggable: CHANGELOG: Call out default-mode in-flow ancestor change The original Draggable entry covered the stacking + cross-document fallback story, but left the load-bearing behavior change for third-party consumers in the PR description only: in the default placement mode (no `appendToOwnerDocument`, no `__experimentalDragComponent`), the clone used to be a DOM descendant of the dragged element's parent. With the slot active, it now lives at the body-level slot regardless. Surface that change directly in the CHANGELOG entry, including a migration hint for consumers that scoped CSS or event delegation on the clone's previous ancestry. * Draggable: Add e2e regression for chip-inside-compat-slot Lock in the structural guarantee that underpins the stacking claim in #78183: when `@wordpress/components`'s `Draggable` runs in a WordPress environment, the drag chip is rendered inside the body-level `[data-wp-compat-overlay-slot]`. That single structural assertion subsumes the visual stacking contract — the slot creates an isolated stacking context with `z-index: 1000000003`, so anything appended into it stacks above any `@wordpress/components` overlay opened mid-drag (which live outside the slot at lower `z-index`s). Asserting structure rather than visual layering keeps the test robust against unrelated overlay z-index churn, and avoids a brittle `elementFromPoint`-style probe across the parent-doc/canvas-iframe boundary. * Storybook: Trim file-level docblocks on playground stories Drop the file-level brain-dump JSDoc from the `draggable-cross-document-fallback` and `popover-with-slotfill` playground stories. The story body and any per-story copy carry the user-facing explanation; the file-level prose was internal reasoning that doesn't belong in the story source. Per mirka's review on PR #78183 (empty-suggestion blocks). * Storybook: popover-with-slotfill story: use public @wordpress/ui API Switch the playground story to consume `Popover` from the public `@wordpress/ui` entry point instead of reaching into `packages/ui/src/popover`. Inline a small `IframePortal` helper locally so the story no longer depends on `packages/ui/src/popover/stories/utils` either (those story utilities are not part of any public surface). Also swap the `Slot` ref from `useRef` to a state setter so the popup re-renders once the slot's container element mounts, which removes a first-render race the previous `useRef` pattern had. Per mirka's review on PR #78183. * Storybook: draggable cross-doc story: load components styles via Storybook bundle Swap the iframe's style injection from a `?inline` import of `packages/components/src/draggable/style.scss` (reaching into another package's source) to Storybook's own `storybook/package-styles/components-ltr.lazy.scss`, which is the canonical bundle of `@wordpress/components` styles for stories. The injected CSS is now broader than strictly necessary (the whole package stylesheet rather than only Draggable's rules), but this is a debug fixture and the cost is negligible. In exchange we drop the cross-package src reach. Per mirka's review on PR #78183. * Storybook: Move cross-document fallback story under "Debug fixtures" The cross-document fallback story is strictly defensive regression coverage and doesn't illustrate a pattern non- maintainers would seek out. Move it under a `Debug fixtures` sub-section in the sidebar so the main `Playground/` namespace stays focused on intended-usage demos. Per mirka's review on PR #78183. * Storybook: Drop redundant `parameters.sourceLink` from playground stories The `source-link` Storybook addon already derives the GitHub source path from `storyData.importPath` when no explicit `parameters.sourceLink` is provided (see `storybook/addons/source-link/manager.ts`). For stories living under `storybook/stories/playground/`, that fallback resolves to the same value the explicit `sourceLink` was hard-coding, so the declaration is pure duplication. Per mirka's review on PR #78183 (empty-suggestion blocks covering the `parameters: { sourceLink: ... }` literal). * Draggable: Migrate styles from SCSS to a CSS module Move the (already small) Draggable stylesheet to a CSS module so its rules travel via `@wordpress/style-runtime` (and therefore into any iframe wrapped in `<StyleProvider>` — e.g. the block-editor canvas) without needing the package-level `build-style/style.css` bundle. Drops the `@use` line from `packages/components/src/style.scss`, following the same shape as the `AlignmentMatrixControl` (#73714/#73757) and `AnglePickerControl` (#73786) migrations. The CSS-module class names are standard (hashed). The legacy `components-draggable__*` / `is-dragging-components-draggable` class names are kept by adding them alongside the hashed ones in the JS `classList.add(...)` calls, since several other Gutenberg packages reference them in their own stylesheets (block-editor's `list-view`, `block-tools`, `block-library`'s `navigation` editor, `edit-widgets`' `widget-area` editor) and block-editor runtime JS reads `is-dragging-components-draggable` off `document.body`. Dropping those names would silently break those consumers. Per mirka's review on PR #78183 (CSS-module option for the iframe story); the corresponding Storybook simplification follows in a separate commit. * Storybook: Simplify cross-document fallback story with StyleProvider Now that Draggable's styles ship as a CSS module routed through `@wordpress/style-runtime`, the cross-document fallback story no longer needs to manually `?inline`-import and inject the whole `components-ltr` SCSS bundle into the iframe's `<head>`. Wrap the portaled iframe content in `<StyleProvider document={iframeDoc}>` from `@wordpress/components` instead — `StyleProvider` calls `registerDocument()` on the iframe document, and the style registry replays every registered CSS module (Draggable included) into that document. The visible behavior is unchanged: the orange clone still tracks the cursor inside the iframe, demonstrating the cross-document fallback. Per mirka's review on PR #78183. * Draggable: CHANGELOG: Move entry to Unreleased and slim it down The Enhancements entry for this PR ended up rolled into the already-cut `33.1.0` release section during an earlier rebase, and had grown to a 700-character paragraph spelling out every edge case (cross-document fallback, `appendToOwnerDocument` semantics, in-flow ancestor migration hints). Move it back to `## Unreleased` and trim to a two-sentence summary in line with the surrounding entries. The dropped detail still lives in the JSDoc, the code comments, and the PR description's <details> blocks. * Draggable: Trim verbose inline code comments Sweep across the comments added by this PR, dropping redundant duplication, narration of self-evident code, and prose that already lives in the PR description / JSDoc: - Drop the duplicate compat-slot note from the `AppendElementToOwnerDocument` story JSDoc (the interaction is already described on the prop's TS JSDoc in `types.ts`). - Tighten the prop JSDoc for `appendToOwnerDocument` to a single short paragraph. - Slim the same-document-only slot guard comment in `Draggable.start()` (the conditional itself reads as "slot if same document"). - Compact the rationale comment for `parameters.docs.story.inline: false` in the Draggable autodocs config to a single explanation. - Trim the structural-stacking assertion comment in the Playwright `draggable-blocks` spec. - Drop the forward-looking "can be removed on a future Stylelint upgrade" note from `CSS_BASELINE_2024_FUNCTIONS`. No behavior change. * @wordpress/ui CHANGELOG: Move #78183 entry to Unreleased The `getWpCompatOverlaySlot()` export bullet was left inside the already-released `## 0.13.0` section when the parallel `@wordpress/components` entry was moved to `## Unreleased`. * Draggable: Keep physical `left` for the invisible drag image Mirroring this offscreen stand-in in RTL has no benefit — either side hides it equally — so revert to the original physical property and silence the logical-properties lint with a targeted comment. * @wordpress/ui CHANGELOG: Trim #78183 entry * @wordpress/ui: Restore unrelated tsconfig change * Storybook: Drop redundant story-name overrides * Draggable: Keep body cursor class global The cursor flip is also triggered by external code (block-editor keyboard drag, etc.) that toggles `is-dragging-components-draggable` directly. Targeting the legacy class globally preserves that flow, which a module-hashed selector silently broke. * Draggable: Guard class arrays against the Jest CSS-module mock `jest-preset-default`'s style mock returns `undefined` for any class, which `classList.add()` would coerce to a literal "undefined" token. Filter falsy entries to keep test DOM clean. * Draggable: Address minor self-review nits - Note why the invisible drag image bypasses the compat slot. - Drop a redundant chip-count assertion in the e2e spec. - Flag the SCSS-only stylelint override pattern explicitly. * Storybook: Group compat-slot fixtures under Debug fixtures Consolidates the manual verification stories (`WP Compat Overlay Slot`, `Popover with SlotFill`) alongside the existing Draggable fixture. * Draggable: Use kebab-case for CSS module class names --- Co-authored-by: ciampo <[email protected]> Co-authored-by: mirka <[email protected]>

Migrates the
AnglePickerColorcomponent from Emotion to astyle.module.scssstyle.I did the migration with the following Cursor prompt:
After reviewing the results, I had to add several additional clarifications: