Skip to content
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.

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

Already on GitHub? Sign in to your account

Rename the color prop to just c #399

Open
chrispcode opened this issue May 19, 2023 · 10 comments
Open

Rename the color prop to just c #399

chrispcode opened this issue May 19, 2023 · 10 comments

Comments

@chrispcode
Copy link

chrispcode commented May 19, 2023

馃挰 Questions and Help

Here is an example:

import React, { ComponentPropsWithoutRef } from 'react';
import styled, { x, type SystemProps } from '@xstyled/styled-components';

type ReactDiv = ComponentPropsWithoutRef<'div'>;

export interface BoxProps extends ReactDiv, SystemProps {}

function Box(props: BoxProps) {
  return (
    <BoxBase {...props} />
  );
}

const BoxBase = styled(x.div)``;

export default Box;

The code above pisses typescript because the react color prop type (string) is different than the xstyled one ( string | ThemeProp<string, Theme>).

image

If I were to omit the color prop from the react div type it would work perfectly fine

type ReactDiv = Omit<ComponentPropsWithoutRef<'div'>, 'color'>;

The question is, wouldn't it be better to rename the xstyled color prop to something else, for example just c, following a similar pattern with w and h?

@chrispcode
Copy link
Author

@probablyup any comments, if you don't like the idea that's fine

@quantizor
Copy link
Collaborator

Personally I prefer the long form of all the props because it's less confusing, albeit somewhat less convenient

@chrispcode
Copy link
Author

chrispcode commented Jun 18, 2023

It's more about not having TS conflict than changing the prop name to something shorter. What is the suggested way of using xstyled when authoring component libraries, like the example above?

@YassienW
Copy link
Contributor

YassienW commented Oct 9, 2023

I simply omit the html color property from the types, it's redundant anyway. I would rather do that than use the short form xstyled props since i think 1 letter variables are terrible

@chrispcode
Copy link
Author

Why do we have w, h, p, m then?

@quantizor
Copy link
Collaborator

Why do we have w, h, p, m then?

Legacy decision. Given that it's been around for a while, hesitant to remove, but don't want to add more of them.

@YassienW
Copy link
Contributor

YassienW commented Nov 7, 2023

Why do we have w, h, p, m then?

Legacy decision. Given that it's been around for a while, hesitant to remove, but don't want to add more of them.

The quicker they go the less painful it will be 馃槃 Realistically though deprecation warnings can be added before completely removing them. Imo replacing them in a code base should be rather straight forward to automate anyway

@jguddas
Copy link
Contributor

jguddas commented Nov 7, 2023

As far as I remember, w and h were added, so we would avoid conflicts with the image height and width attribute.

@chrispcode
Copy link
Author

chrispcode commented Nov 14, 2023

As far as I remember, w and h were added, so we would avoid conflicts with the image height and width attribute.

How was that request different than the current one? Is it not conflicting with the native tags' color prop. If the user uses the c prop then the components expects a SystemProp type, otherwise it expects a html color prop (string).

@chrispcode
Copy link
Author

Also the request doesn't revolve around renaming the prop necessarily to c. It could be colour or hue for example. As long it does not introduce type conflicts, personally I don't mind.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants