Skip to content

Components: Start using CSS Modules #103004

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

Closed
mirka wants to merge 2 commits into from
Closed

Components: Start using CSS Modules #103004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e170fe5
Components: Start using CSS Modules
mirka Apr 30, 2025
84fa419
Improve types
mirka Apr 30, 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
8 changes: 6 additions & 2 deletions packages/components/src/logos/wordpress-logo/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import './style.scss';
import clsx from 'clsx';
import styles from './style.module.scss';

type Props = {
/**
Expand All @@ -17,7 +18,10 @@ type Props = {
};

export const WordPressLogo: React.FunctionComponent< Props > = ( {
className = 'wordpress-logo',
// TODO: This behavior of the `className` prop overriding the default classes are not standard,
// and should be normalized to be merged instead. Ideally, we also remove the `wordpress-logo`
// class because it clashes very easily.
Comment on lines +22 to +23
Copy link
Member

@aduth aduth Apr 30, 2025

Choose a reason for hiding this comment

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

Ideally, we also remove the wordpress-logo class because it clashes very easily.

With the eventual removal of wordpress-logo, what's the desired end-state where a component is currently overriding styles of .wordpress-logo ? Would that component be expected to pass its own className to reference it in its styles?

Copy link
Contributor

@ciampo ciampo Apr 30, 2025

Choose a reason for hiding this comment

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

Removing the hardcoded .wordpress-logo would be a breaking change, although I'm ok with it if communicated well.

Would that component be expected to pass its own className to reference it in its styles?

Yes, that's my expectation

className = clsx( styles.wordpressLogo, 'wordpress-logo' ),
size = 72,
} ) => {
return (
Expand Down
2 changes: 1 addition & 1 deletion ...nents/src/logos/wordpress-logo/style.scss → ...rc/logos/wordpress-logo/style.module.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
.wordpress-logo {
Copy link
Member Author

@mirka mirka Apr 30, 2025

Choose a reason for hiding this comment

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

You may have noticed that there's a redundant copy of this component in calypso/components 😇 #103012

Copy link
Member

@tyxla tyxla May 1, 2025

Choose a reason for hiding this comment

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

Let's kill it with fire! A migration is a matter of a single Cursor prompt these days. The v2 hosting dashboard eslint rules might be an exception that will need a manual hand.

.wordpressLogo {
Copy link
Member Author

@mirka mirka Apr 30, 2025

Choose a reason for hiding this comment

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

Kebab casing also works, but camel case is the official recommendation.

Copy link
Member

@aduth aduth Apr 30, 2025

Choose a reason for hiding this comment

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

Kebab casing also works, but camel case is the official recommendation.

It's an interesting topic. It definitely feels more natural in JavaScript to reference with a camelCase property, and ultimately the class names are clobbered before they get to the client so it doesn't matter there.

That said, with all the inherited experience of BEM or general kebab-case styling in CSS, I wonder if this will be an issue for people, especially in transitioning between files with different conventions. Are there ways we can make this easier, or enforce some consistency within respective file types? Thinking documentation or tooling like Stylelint rules.

I also personally don't have a huge issue with referencing the kebab-case properties to keep consistency, even if it's a little less ergonomic.

Copy link
Member Author

@mirka mirka Apr 30, 2025

Choose a reason for hiding this comment

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

No strong opinion on my end. The ergonomics/safety characteristics aren't that different once you're using an IDE extension or the ts plugin.

One argument in favor of camel case is that it'll be more obvious that the file is a module, not a horrible BEM file 😂

Copy link
Member

@tyxla tyxla May 1, 2025

Choose a reason for hiding this comment

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

I have a pretty strong opinion to keep using kebab case, at least until it's the official recommendation of the WordPress CSS Coding Standards

fill: var(--color-text-inverted);
margin: 0 auto 20px;
}
10 changes: 10 additions & 0 deletions packages/components/src/types.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,13 @@ declare module '*.svg' {
}

declare const __i18n_text_domain__: string;

declare module '*.module.css' {
Copy link
Member Author

@mirka mirka Apr 30, 2025

Choose a reason for hiding this comment

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

Keeping this to only @automattic/components for now, but I think we'll also want it in at least client/ as well.

const classes: { [ key: string ]: string };
export default classes;
}

declare module '*.module.scss' {
const classes: { [ key: string ]: string };
export default classes;
}