Skip to content
This repository has been archived by the owner on Mar 14, 2021. It is now read-only.

Add aliases to colors palettes #13

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

frycz
Copy link

@frycz frycz commented Apr 6, 2020

resolves #11

@@ -1,7 +1,7 @@
Renders a color swatch with a readable text.

```jsx harmony
<ColorSwatch token={"accent"} value={"#C0100F"} />
<ColorSwatch token={"accent"} aliases={"error"} value={"#C0100F"} />
Copy link
Member

Choose a reason for hiding this comment

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

Let's create a separate example for aliases. Also the name suggests it should be an array but it's a string. I also don't see any propTypes definitions for that prop.

@@ -1,5 +1,5 @@
```jsx harmony
import theme from "../theme";

<Colors theme={theme} />;
<Colors theme={theme} aliasesKey={"aliases"} />;
Copy link
Member

Choose a reason for hiding this comment

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

Same here. Leave the basic example and add an advanced one where you explain why it's like this. I don't like the aliasesKey prop name TBH since it's not self explanatory.. Again, prop types are missing.

Comment on lines +17 to +29
palette.aliases = [
"lightest",
"light",
"diabled",
"inactive",
"regular",
"hovered",
"errorLight",
["error", "failure"],
"errorDark"
];

<PaletteSwatch token={"red"} value={palette} aliasesKey="aliases" />;
Copy link
Member

Choose a reason for hiding this comment

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

This example is overwhelming for me. Here is an example from the spec: https://styled-system.com/theme-specification#space which I'd suggest using.

Comment on lines +13 to +20
aliases: {
darker: "foused",
dark: "active",
base: "default",
light: "hovered",
lighter: "disabled",
lightest: "inactive"
}
Copy link
Member

Choose a reason for hiding this comment

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

Why did that change?

Comment on lines +7 to +22
? items.map((value, index) =>
children(
index,
value,
aliasesKey && items[aliasesKey] ? items[aliasesKey][index] : null
)
)
: Object.keys(items)
.filter(key => key !== aliasesKey)
.map(key =>
children(
key,
items[key],
aliasesKey && items[aliasesKey] ? items[aliasesKey][key] : null
)
)}
Copy link
Member

Choose a reason for hiding this comment

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

TBH I'm not sure I want to accept this code into the repo. Maybe the best course of action would be to do this in your repo after all. You can compose things with Swatches and Swatch, add the resolution logic on top of what'e being done and extend provided primitives.

Thoughts?

Copy link
Author

Choose a reason for hiding this comment

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

Maybe we can just use this repo to create a custom library? The only component which can be reused without any change is Swatch (which gives copy-to-clipboard feature)

Copy link
Member

@okonet okonet Apr 7, 2020

Choose a reason for hiding this comment

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

I'm not sure yet. I'd like to support aliases but I'm not sure the code complexity is worth it. Is there a simpler way of getting aliases for tokens? Ideally, as the user, I should not care about providing alias keys to the library. Let me poke with it a bit and let you know.

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

Successfully merging this pull request may close these issues.

Support aliases for colors
2 participants