The "getSelector" API feels strange

[oliviertassinari]

Steps to reproduce

I'm confused by this getSelector API:

1. What does the name mean? The name feels obscure. How about we rename it to something more explicit? Looking at the source, it seems to be close to a getColorSchemeSelector so maybe the name should be merged. Now, I imagine we shouldn't confuse it with the helper developer would use to write conditional logic, e.g. theme.applyStyles('dark', {}).

pigment-css/packages/pigment-css-react/src/utils/extendTheme.ts

Lines 20 to 35 in e672e4b

	/**
	* If provided, it will be used to create a selector for the color scheme.
	* This is useful if you want to use class or data-* attributes to apply the color scheme.
	*
	* The callback receives the colorScheme with the possible values of:
	* - undefined: the selector for tokens that are not color scheme dependent
	* - string: the selector for the color scheme
	*
	* @example
	* // class selector
	* (colorScheme) => colorScheme !== 'light' ? `.theme-${colorScheme}` : ":root"
	*
	* @example
	* // data-* attribute selector
	* (colorScheme) => colorScheme !== 'light' ? `[data-theme="${colorScheme}"`] : ":root"
	*/

pigment-css/packages/pigment-css-react/src/utils/extendTheme.ts

Lines 150 to 155 in e672e4b

	finalTheme.getColorSchemeSelector = (colorScheme: string) => {
	if (!theme.getSelector) {
	return `@media (prefers-color-scheme: ${colorScheme})`;
	}
	return `:where(${theme.getSelector(colorScheme, {})}) &`;
	};

2. I don't resonate with the API shape. I would expect 90% of the people not to use the media query approach. I mean, I would personally always make this selectors-based, media query is for the reallllllly quick and dirty websites, but has no purpose otherwise IMHO. If this is how people behave, it means that most people will need to reimplement their own custom selectors like getSelector: (colorScheme) => colorScheme ? `.theme-${colorScheme}` : ':root',. So, wouldn't it be better to match Tailwind CSS API? https://tailwindcss.com/docs/dark-mode#toggling-dark-mode-manually. I would propose something like this:

interface PigmentConfig {
  /**
   * @default media
   */
  colorScheme: 'selector' | 'media';
  /**
   * @default `getSelector: (colorScheme) => colorScheme ? `:where(.theme-${colorScheme})` : ':root',`
   */
  getColorSchemeSelector: (colorScheme: string) => string;
}

Context

As a side note:

• a. It looks like this whole function is dead code, to remove, no?

pigment-css/packages/pigment-css-react/src/utils/extendTheme.ts

Line 104 in e672e4b

function defaultGetSelector(

• b. The apps are wrong, no? The return type is an object:

pigment-css/apps/pigment-css-next-app/next.config.js

Line 84 in e672e4b

getSelector: function getSelector(colorScheme, css) {

but the expected type is a string:

pigment-css/packages/pigment-css-react/src/utils/extendTheme.ts

Line 154 in e672e4b

return `:where(${theme.getSelector(colorScheme, {})}) &`;

• c. The docs look wrong to me: it should be :where() otherwise, style will override :hover, :focus-visible, etc styles.

pigment-css/README.md

Line 667 in e672e4b

+ getSelector: (colorScheme) => colorScheme ? `.theme-${colorScheme}` : ':root',

This makes the case for why we need 1.

Your environment

npx @mui/envinfo

  Don't forget to mention which browser you used.
  Output from `npx @mui/envinfo` goes here.

Search keywords: -

[siriwatknp]

Absolutely, it can follow Tailwind API. Will change the default selector to use class approach.

As a note: Tailwind default behavior is media.

[oliviertassinari]

Tailwind default behavior is media.

I think this is great, I would advocate to do the same so it works in the configuration-less mode.

[paulm17]

ignore me, getSelector via extendedTheme works perfectly.