Skip to content
This repository has been archived by the owner on Feb 13, 2023. It is now read-only.

Allow custom colors with isColor #85

Open
Heanthor opened this issue Feb 3, 2018 · 5 comments
Open

Allow custom colors with isColor #85

Heanthor opened this issue Feb 3, 2018 · 5 comments

Comments

@Heanthor
Copy link
Contributor

Heanthor commented Feb 3, 2018

I noticed that the isColor prop only works with built-in colors like primary. But by customizing bulma's $colors variable, you can use new colors with the is- prefix.

For example:

$colors: (
   myColor: (#ffffff, #000000)
);

will style this correctly

<div class="is-myColor">

but this does not work in Bloomer

<Notification isColor="myColor">

Does this feature make sense to add? Right now, I'm having to manually set the colors like so

<Notification className="is-myColor">

which removes some of the convenience of this awesome library.

@AlgusDark
Copy link
Owner

AlgusDark commented Feb 3, 2018 via email

@Heanthor
Copy link
Contributor Author

Heanthor commented Feb 3, 2018

I haven't worked with typescript before, but I could certainly learn a little and try to contribute.

So you're suggesting a file like colors.ts that the user adds to define their own colors, instead of using scss? Then just check if it's defined in the main bulma.ts?

@AlgusDark
Copy link
Owner

AlgusDark commented Feb 4, 2018

All colors are in bulma.tsx Colors type. Maybe we can implement a default for isColor that is used in getColorModifiers like:

export function getColorModifiers({ isColor: color }: Bulma.Color) {
    return [`is-${color}`]: true;
}

That way we can have Typescript intellisense (only for default ones) and we can allow new colors. The best solution should be to extend Color interface to have the custom colors in the intellisense, but I think is a little Overengineering for now. If you think of a better approach, I'm open to discuss :)

This approach is better for this Bloomer Version, and what I was suggesting (a file like colors.ts) is going to be better approach for the new Bloomer Version since we would allow the override of almost anything as if we were using SASS.

Can you try the snippet I showed you and test? I can check your PR if everything is good :)

PS: You had a really good understanding on the suggestion, but I believe that it's going to be a pain to do it and it's going to force people to keep track of several stuff right now.

@Heanthor
Copy link
Contributor Author

Heanthor commented Feb 5, 2018

So you're suggesting we remove the isColor check from getColorModifiers to allow any color to work with the function, in which case would it look like this? :

export function getColorModifiers({ isColor: color }: Bulma.Color) {
    return  { [`is-${color}`]: true };
}

I understand now why I'll need to add a default to the Colors type to let me pass an arbitrary string as a Color. This change seems to make the isColor function unused, but I suppose that's okay.

Might we also want to modify getColorModifier in the same way?

I think for now this is a good solution, if your new version is on its way there seems to be less incentive to make a more involved addition.

If this sounds good I'll gladly update the tests and PR it. Might have a few more questions as I'm going 🙂

@AlgusDark
Copy link
Owner

Yes, I believe that the best way should be to remove the isColor constraint and accept strings, at the end this way we can have the "default" as a Color. So we should remove isColor and make the functions receive color type (like we do now) and return whatever the user pass.

So, if a user pass something like <Button isColor="custom">, the function getColorModifiers is going to return "is-custom" to Button component and problem solve.

We should check for all isColor implementations, like the one with has-text-${modifier}. At first I was doing all these to help the user so it was rare to have errors, but we need to leave the user the power to do anything since at the end, if we do something like <Component className="custom"> and we don't have .custom in our css, React renders our Component with that className.

So, check for those constrains, maybe we can modify another constraints until the release of the new Bloomer Library 😃

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

No branches or pull requests

2 participants