Skip to content

AnglePickerColor: migrate Emotion to style.module #73786

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jsnajdr wants to merge 5 commits into
base: trunk
Choose a base branch
from
+76 −77
Open

AnglePickerColor: migrate Emotion to style.module #73786

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f76881d
AnglePickerColor: migrate Emotion to style.module
jsnajdr Dec 5, 2025
4203b46
Simplify degree suffix
mirka Dec 5, 2025
169ecc3
Add code comment
mirka Dec 5, 2025
7f27a3a
Bring back named classes
jsnajdr Dec 8, 2025
12a907a
Add changelog entry
jsnajdr Dec 8, 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
27 changes: 15 additions & 12 deletions packages/components/src/angle-picker-control/angle-circle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,8 @@
/**
* External dependencies
*/
import clsx from 'clsx';

/**
* WordPress dependencies
*/
Expand All @@ -7,11 +12,7 @@ import { __experimentalUseDragging as useDragging } from '@wordpress/compose';
/**
* Internal dependencies
*/
import {
CircleRoot,
CircleIndicatorWrapper,
CircleIndicator,
} from './styles/angle-picker-control-styles';
import styles from './style.module.scss';

import type { WordPressComponentProps } from '../context';
import type { AngleCircleProps } from './types';
Expand All @@ -25,6 +26,7 @@ type UseDraggingCallbackEvent =
function AngleCircle( {
value,
onChange,
className,
...props
}: WordPressComponentProps< AngleCircleProps, 'div' > ) {
const angleCircleRef = useRef< HTMLDivElement | null >( null );
Expand Down Expand Up @@ -89,22 +91,23 @@ function AngleCircle( {
}, [ isDragging ] );

return (
<CircleRoot
// eslint-disable-next-line jsx-a11y/no-static-element-interactions
<div
ref={ angleCircleRef }
onMouseDown={ startDrag }
className="components-angle-picker-control__angle-circle"
Copy link
Member Author

@jsnajdr jsnajdr Dec 5, 2025

Choose a reason for hiding this comment

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

All the components-angle-picker-control-* class names are unused, so I removed them.

Copy link
Member

@mirka mirka Dec 5, 2025

Choose a reason for hiding this comment

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

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

<DayInput
key="day"
className="components-datetime__time-field components-datetime__time-field-day" // Unused, for backwards compatibility.

Copy link
Member Author

@jsnajdr jsnajdr Dec 8, 2025

Choose a reason for hiding this comment

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

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 🙂

className={ clsx( styles[ 'circle-root' ], className ) }
{ ...props }
>
<CircleIndicatorWrapper
<div
style={
value ? { transform: `rotate(${ value }deg)` } : undefined
}
className="components-angle-picker-control__angle-circle-indicator-wrapper"
className={ styles[ 'circle-indicator-wrapper' ] }
tabIndex={ -1 }
>
<CircleIndicator className="components-angle-picker-control__angle-circle-indicator" />
</CircleIndicatorWrapper>
</CircleRoot>
<div className={ styles[ 'circle-indicator' ] } />
</div>
</div>
);
}

Expand Down
5 changes: 3 additions & 2 deletions packages/components/src/angle-picker-control/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,9 @@ import { isRTL, __ } from '@wordpress/i18n';
import { Flex, FlexBlock } from '../flex';
import { Spacer } from '../spacer';
import NumberControl from '../number-control';
import { Text } from '../text';
import AngleCircle from './angle-circle';
import { UnitText } from './styles/angle-picker-control-styles';
import styles from './style.module.scss';

import type { WordPressComponentProps } from '../context';
import type { AnglePickerControlProps } from './types';
Expand Down Expand Up @@ -48,7 +49,7 @@ function UnforwardedAnglePickerControl(

const classes = clsx( 'components-angle-picker-control', className );

const unitText = <UnitText>°</UnitText>;
const unitText = <Text className={ styles[ 'unit-text' ] }>°</Text>;
Copy link
Member

@Mamaduka Mamaduka Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the default Text styles are overriding the unit text styles. The unit symbol is touching the border in this PR>

Screenshot

CleanShot 2025-12-05 at 15 31 38

Copy link
Member Author

@jsnajdr jsnajdr Dec 5, 2025

Choose a reason for hiding this comment

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

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:

Screenshot 2025-12-05 at 12 36 35 Screenshot 2025-12-05 at 12 36 50

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.

Copy link
Member

@Mamaduka Mamaduka Dec 5, 2025

Choose a reason for hiding this comment

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

Is the something fixable? Don’t like the idea of relying on random order.

Copy link
Member

@mirka mirka Dec 5, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@jsnajdr jsnajdr Dec 5, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@jsnajdr jsnajdr Dec 5, 2025

Choose a reason for hiding this comment

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

I think we could remove the Text component and the custom CSS styles, and just use InputControlSuffixWrapper for 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.

Copy link
Member

@mirka mirka Dec 5, 2025

Choose a reason for hiding this comment

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

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.

const [ prefixedUnitText, suffixedUnitText ] = isRTL()
? [ unitText, null ]
: [ null, unitText ];
Expand Down
46 changes: 46 additions & 0 deletions packages/components/src/angle-picker-control/style.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
@use "@wordpress/base-styles/variables" as *;
@use "../utils/theme-variables" as *;

.circle-root {
border-radius: $radius-round;
border: $border-width solid $components-color-border;
box-sizing: border-box;
cursor: grab;
height: 32px;
overflow: hidden;
width: 32px;

&:active {
cursor: grabbing;
}
}

.circle-indicator-wrapper {
box-sizing: border-box;
position: relative;
width: 100%;
height: 100%;

&:focus-visible {
outline: none;
}
}

.circle-indicator {
background: $components-color-accent;
border-radius: $radius-round;
box-sizing: border-box;
display: block;
left: 50%;
top: 4px;
transform: translateX(-50%);
position: absolute;
width: 6px;
height: 6px;
}

.unit-text {
color: $components-color-accent;
margin-right: $grid-unit-15;
}

58 changes: 0 additions & 58 deletions packages/components/src/angle-picker-control/styles/angle-picker-control-styles.tsx
View file
Open in desktop

This file was deleted.

Loading