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

Typed Css values #409

Closed
wants to merge 1 commit into from
Closed

Typed Css values #409

wants to merge 1 commit into from

Conversation

owanturist
Copy link

Hello, first of al thanks for the package - it's a really huge work.

Problem 1: access to not exposed Compatible

When I was testing this package first time I realised that developers able to use some values like this:

myCustomColor: Css.ColorValue {}
myCustomColor =
	{ value = "some invalid color", color = Css.transparent.value }

I think it's unexpected using of the package and something has to change the situation.

Problem 2: huge value records

Each value of css rule is described as type alias with dynamic record with Compatible. It means that result record should contain same structure as type alias:

auto :
    { value : String
    , lengthOrAuto : Compatible
    , overflow : Compatible
    , textRendering : Compatible
    , flexBasis : Compatible
    , lengthOrNumberOrAutoOrNoneOrContent : Compatible
    , alignItemsOrAuto : Compatible
    , justifyContentOrAuto : Compatible
    , cursor : Compatible
    , lengthOrAutoOrCoverOrContain : Compatible
    , intOrAuto : Compatible
    , pointerEvents : Compatible
    , touchAction : Compatible
    , tableLayout : Compatible
    }
auto =
    { value = "auto"
    , lengthOrAuto = Compatible
    , overflow = Compatible
    , textRendering = Compatible
    , flexBasis = Compatible
    , lengthOrNumberOrAutoOrNoneOrContent = Compatible
    , alignItemsOrAuto = Compatible
    , justifyContentOrAuto = Compatible
    , cursor = Compatible
    , lengthOrAutoOrCoverOrContain = Compatible
    , intOrAuto = Compatible
    , pointerEvents = Compatible
    , touchAction = Compatible
    , tableLayout = Compatible
    }

It looks little bit redundantly.

Decision

We can solve both problems by one decision:

type Value compatible
    = Value String

Only the Value type should be exposed and as result we will get something like this:

auto :
    Value
        { lengthOrAuto : Compatible
        , overflow : Compatible
        , textRendering : Compatible
        , flexBasis : Compatible
        , lengthOrNumberOrAutoOrNoneOrContent : Compatible
        , alignItemsOrAuto : Compatible
        , justifyContentOrAuto : Compatible
        , cursor : Compatible
        , lengthOrAutoOrCoverOrContain : Compatible
        , intOrAuto : Compatible
        , pointerEvents : Compatible
        , touchAction : Compatible
        , tableLayout : Compatible
        }
auto =
    Value "auto"

Advantages

  1. Developers don't have access to Compatible by getting value of record field so they can't create custom css values without using API of Css
  2. Value records is gone and each css value should be described as Value String.
  3. Type guard still works.

Disadvantages

  1. Specific fields cannot be passed by record because it works only for type checking (numericValue and unitLabel)
    type alias Length compatible units =
        { compatible
            | value : String
            , length : Compatible
            , numericValue : Float
            , units : units
            , unitLabel : String
        }
    But I'm not sure that this point is bad because we should do it in fact.
  2. Types of css values cannot be nested:
    content : LengthOrNumberOrAutoOrNoneOrContent (FlexBasis {})
    content =
        { value = "content"
        , flexBasis = Compatible
        , lengthOrNumberOrAutoOrNoneOrContent = Compatible
        }

Conclusion

There are a lot of changes but most of them looks like changing of records to Value String. Some exposed types has been removed/added and that will increase package version. I don't familiar with your politic of version management and hope that it won't be a big deal. There are big changes of working with colours (it was simplified in general).

@rtfeldman I'm looking forward to your feedback.

Typed value
@rtfeldman
Copy link
Owner

rtfeldman commented Apr 22, 2018

Hi @owanturist,

Have you taken a look at #375 or #392 yet?

@owanturist
Copy link
Author

owanturist commented Apr 22, 2018

@rtfeldman thank you for fast response!

Oops, I haven't.. it's my fail.. I wanted to make something useful too much but didn't read any issues.

I've just read the issues and realised three things:

  1. We thought on the same way independently and I really happy with it
  2. I should make this work step by step with community by guideline instead of making single huge PR with tons of code and ignoring some points of guideline
  3. Your idea is working in fact when whole module will be changed to new approach

I think the PR can be closed and I'm going to contribute by "legal" flow.

@rtfeldman rtfeldman closed this Apr 22, 2018
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

Successfully merging this pull request may close these issues.

None yet

2 participants