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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add string overloads to CSS properties for raw custom values #109

Open
Shmew opened this issue Nov 11, 2019 · 15 comments
Open

Add string overloads to CSS properties for raw custom values #109

Shmew opened this issue Nov 11, 2019 · 15 comments
Labels
enhancement New feature or request Feliz Issues related to Feliz good first issue Good for newcomers

Comments

@Shmew
Copy link
Collaborator

Shmew commented Nov 11, 2019

Finally getting around to playing with Feliz itself more. Noticed a few things that should/could be changed:

Some properties are too strict. For example if I'm using Material-UI and want to set box-shadow I'm not able to set it through boxShadow as the overloads only accept int and int tuples. MaterialUI theme.shadows is an array of strings and the general usage is like this:

Styles.create [ style.custom ("box-shadow", (theme.shadows.[5])) ]

I'm not sure if this should be adjusted to just have all style props be able to accept a string or just make them do style.custom when doing this kind of setting. @cmeeren thoughts?

I've also noticed outline and justify-content is completely missing.

@cmeeren
Copy link
Contributor

cmeeren commented Nov 11, 2019

Sorry, I'm lost. Basic CSS props have nothing to do with MUI. Feliz is right to model them according to the CSS spec. When you say MUI uses an array of strings, what do you mean? AFAIK CSS has no concept of an array. Could you be mixing CSS props with component props?

@Shmew
Copy link
Collaborator Author

Shmew commented Nov 11, 2019

Well it's just an array of strings that I'm accessing by index like in the above snippet. Yeah I agree that Feliz shouldn't specifically take into account a library. My question was if things should fall back to allow string input for those props.

@cmeeren
Copy link
Contributor

cmeeren commented Nov 11, 2019

Oh, right. You're referring to this:

image

Yes, perhaps Feliz's boxShadow should be loosened?

By the way, see this page. I have no idea what they mean by it, but perhaps there's another way to use the theme's shadows array in MUI, by specifying the index in a boxShadow property (not style)?

@Shmew
Copy link
Collaborator Author

Shmew commented Nov 11, 2019

Doesn't look like fade supports that prop unfortunately, which is what I'm using that particular property for.

@cmeeren
Copy link
Contributor

cmeeren commented Nov 11, 2019

I haven't seen that prop for any components at all, and neither have I seen the Box component they use on the page I linked too, which is why I'm confused. Just for the sake of it, you could try setting a custom "boxShadow" or "box-shadow" prop on any component and see what works.

In any case, the "shadow" docs page should really be improved.

@Zaid-Ajaj
Copy link
Owner

Yes, perhaps Feliz's boxShadow should be loosened?

Ideally it shouldn't have to because that is what style.custom is for. Because this is react we talking about, I think it should be boxShadow. Though again, a string overload shouldn't be harmful. I will think about it

@Shmew
Copy link
Collaborator Author

Shmew commented Nov 12, 2019

Having a base string overload for everything lets people do the same as style.custom without having to go lookup/remember what the actual css name is. An option would be to have something like style.boxShadow.custom so they still know they're wandering off the typical use cases for that style.

@cmeeren
Copy link
Contributor

cmeeren commented Nov 12, 2019

Having a base string overload for everything lets people do the same as style.custom without having to go lookup/remember what the actual css name is.

Hm, not a bad idea, actually. I mean, we definitely want to encourage type safety. But in CSS everything's a string anyway, and it would be nice to have an escape hatch sometimes for "quick and dirty" stuff, or, as @Shmew says, if something is not currently supported.

I'm not convinced it's a good idea to have string overloads for all styles attributes, but it's something to consider, at least.

@zanaptak
Copy link
Contributor

How about all known style names attached to style.custom to avoid having to give overloads to individual styles.

style.custom.boxShadow "3px 3px red, -1em 0 0.4em olive"
style.custom.transition "margin-right 4s ease-in-out 1s"
style.custom("-webkit-text-stroke", "4px navy") // keep original custom fallback too

@Shmew
Copy link
Collaborator Author

Shmew commented Nov 12, 2019

I'm not convinced it's a good idea to have string overloads for all styles attributes, but it's something to consider, at least.

@cmeeren Using any outside library that could provide data to a style is going to be a hassle when they're intended to be provided directly to the style. We either have to extract the data or always use style.custom ("..", someLibraryThing)

How about all known style names attached to style.custom to avoid having to give overloads to individual styles.

@zanaptak I like this idea too, I'm not sure if that's better or worse than what I suggested for discoverability.

@cmeeren
Copy link
Contributor

cmeeren commented Nov 12, 2019

Using any outside library that could provide data to a style is going to be a hassle when they're intended to be provided directly to the style

True.

I think the simplest solution is to simply have a string overload for all style props. The string parameter could be called rawValue or similar to make it clear that you're on your own.

@Zaid-Ajaj
Copy link
Owner

I think the simplest solution is to simply have a string overload for all style props. The string parameter could be called rawValue or similar to make it clear that you're on your own.

@cmeeren I think this one is good and is a natural extension of the provided overloads but I worry that if we just use style.boxShadow(rawValue: string) it will be abused a lot instead of writing the code in the recommended type-safe manner. I also like the approach provided by @zanaptak because it discourages the use of raw values for CSS unless you know what you are doing and there is no type-safe way of writing the CSS. But again, in the use case above when interoping with a external CSS, using a custom value shouldn't feel like a hack or workaround so style.boxShadow(rawValue: string) and I would imagine more people would want this level of ease when writing the custom values.

Both approaches are good and have good reasons. Personally I am leaning more towards the former approach of style.boxShadow(rawValue: string).

Sorry I haven't been able to discuss/fix many issues around Feliz/Feliz.Mui this week because of work, I try to get back to these issues ASAP and pick this one up too (unless someone wants to take it, would be awesome)

@Zaid-Ajaj Zaid-Ajaj changed the title Some CSS properties missing/Too strict Add string overloads to CSS properties for raw custom values Nov 13, 2019
@cmeeren
Copy link
Contributor

cmeeren commented Nov 13, 2019

it will be abused a lot instead of writing the code in the recommended type-safe manner

Feliz exists for developer happiness. If developers are more happy abusing it than using the type-safe overloads - not that I necessarily think they are - should we really stop them?

In any case, it seems we're all more or less agreein on a rawValue: string overload for all style props. :)

@Zaid-Ajaj
Copy link
Owner

Feliz exists for developer happiness. If developers are more happy abusing it than using the type-safe overloads - not that I necessarily think they are - should we really stop them?

I agree, is not a problem really but I want to be careful to add too many things that let you shoot yourself in the foot. In this context it is definitely not the case.

@cmeeren
Copy link
Contributor

cmeeren commented Nov 13, 2019

I want to be careful to add too many things that let you shoot yourself in the foot.

Absolutely with you on that one 👍

@Zaid-Ajaj Zaid-Ajaj added enhancement New feature or request Feliz Issues related to Feliz good first issue Good for newcomers labels Nov 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request Feliz Issues related to Feliz good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants