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

Phantom border radius #413

Merged
merged 11 commits into from
May 2, 2018

Conversation

owanturist
Copy link

@owanturist owanturist commented Apr 26, 2018

Hi @rtfeldman! The next bulk of properties as a part of #392 are here 😀 Please take a look.

Checklist

  • Each Value is an open record with a single field. The field's name is the value's name, and its type is Supported. For example foo : Value { provides | foo : Supported }
  • Each function returning Style accepts a closed record of Supported fields.
  • If a function returning Style takes a single Value, that Value should always support inherit, initial, and unset because all CSS properties support those three values! For example, borderFoo : Value { foo : Supported, bar : Supported, inherit : Supported, initial : Supported, unset : Supported } -> Style
  • If a function returning Style takes more than one Value, however, then none of its arguments should support inherit, initial, or unset, because these can never be used in conjunction with other values! For example, border-radius: 5px 5px; is valid CSS, border-radius: inherit; is valid CSS, but border-radius: 5px inherit; is invalid CSS. To reflect this, borderRadius : Value { ... } -> Style must have inherit : Supported in its record, but borderRadius2 : Value { ... } -> Value { ... } -> Style must not have inherit : Supported in either argument's record. If a user wants to get border-radius: inherit, they must call borderRadius, not borderRadius2!
  • Every exposed value has documentation which includes at least 1 code sample.
  • Documentation links to to a CSS Tricks article if available, and MDN if not.
  • Make a pull request against the phantom-types branch, not master!

Copy link
Author

@owanturist owanturist left a comment

Choose a reason for hiding this comment

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

There are some points where I need @rtfeldman opinion for making a decision about some changes.

src/Css.elm Outdated

**Note:** An implementation of shorthand property with supporting of elipse shape looks ugly,
so if you wanna make somehting like this `border-radius: 4px 3px 6px / 2px 4px` by `elm-css`
you should create a batch of `Style`:
Copy link
Author

Choose a reason for hiding this comment

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

I didn't know how to say about the implementation problem more polite.. and also my English is not so cool as should be and I need your help as a native speaker 😅

Copy link
Owner

@rtfeldman rtfeldman Apr 27, 2018

Choose a reason for hiding this comment

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

Wow, I never knew about / in border-radius!

I wonder if we should consider supporting this. We could do that by adding something like , ellipticalRadius : Supported to each of the Value arguments in borderRadius, then introducing this:

ellipticalCorner :
    Value { px : Supported, em : Supported, ... }
    -> Value { px : Supported, em : Supported, ... }
    -> Value { provides | ellipticalCorner : Supported }

so then you could call:

borderRadius (ellipticalCorner (px 10) (px 30))

thoughts @owanturist?

Copy link
Author

@owanturist owanturist Apr 27, 2018

Choose a reason for hiding this comment

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

The main problem is specific scheme of this property. I'd like to see the scheme like this:

border-radius: 1px/2px 3px 4px/5px 6px/7px;

Where 1px, 3px, 4px and 6px are general values (like in border-radius: 1px 3px 4px 6px) and rest (2px, 5px and 7px) are addition values.

But it's a wrong signature and correct equivalent of example is:

border-radius: 1px 3px 4px 6px / 2px 0 5px 7px;

So, you can see that supporting of / in borderRadius is not trivial, because all of them are valid:

border-radius: 1px / 2px;
border-radius: 1px / 2px 3px;
border-radius: 1px / 2px 3px 4px;
border-radius: 1px / 2px 3px 4px 5px;

border-radius: 1px 2px / 3px;
border-radius: 1px 2px / 3px 4px;
border-radius: 1px 2px / 3px 4px 5px;
border-radius: 1px 2px / 3px 4px 5px 6px;

border-radius: 1px 2px 3px / 4px;
border-radius: 1px 2px 3px / 4px 5px;
border-radius: 1px 2px 3px / 4px 5px 6px;
border-radius: 1px 2px 3px / 4px 5px 6px 7px;

/* end so on... */

Well, if we wanna implement ellipticalCorner (which can be reused in font property if the rule would looks like border-radius: 1px/2px 3px 4px/5px 6px/7px) we could implement borderRadius from single argument to nine (borderRadius9 (px 1) (px 2) (px 3) (px 4) ellipticalCorner (px 5) (px 6) (px 7) (px 8) is most long) or pass tuple of list to ellipticalCorner but in this case we loose type checking.

But even in borderRadius9 we can't guarantee that developer pass ellipticalCorner in right place (borderRadius9 (px 1) ellipticalCorner (px 2) (px 3) (px 4) (px 5) (px 6) (px 7) (px 8) for example) so we should implement the property borderRadius4elliptical4 for correct type checking:

borderRadius4elliptical4 (px 1) (px 2) (px 3) (px 4) (px 5) (px 6) (px 7) (px 8)

@rtfeldman sorry for this long story, but yesterday I broke my head while I was thinking about it 🤯

src/Css.elm Outdated

**Note:** As with any shorthand property, individual sub-properties cannot inherit,
such as in `borderRadius2 (px 1) inherit`, which would partially override existing definitions.
Instead, the individual longhand properties have to be used.
Copy link
Author

Choose a reason for hiding this comment

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

There is a limitation which I had never known (and I'm ashamed about it). I'm not sure that we should encapsulate things like this but the point is easy to do. So I've removed inherit: Supported for each of borderRadius{2,3,4} and border{TopLeft,TopRight,BotomRight,BottomLeft}Radius2 and keep for same single-argument properties. What do you thing about it? Should we handle the limitation here or developers should do it self?

Copy link
Owner

Choose a reason for hiding this comment

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

Oh wow, and it's true of initial and unset too! 😮

Great catch @owanturist! I think we should definitely change the other properties to reflect this, and I updated the contributing docs to note this: https://github.com/rtfeldman/elm-css/wiki/Phantom-Types:-Contributing

Copy link
Author

Choose a reason for hiding this comment

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

I didn't check it for initial and unset but you're right! I think we should reflect this.

src/Css.elm Outdated
}
-> Value { properties | elliptical : Supported }
elliptical (Value radius) =
Value (" / " ++ radius)
Copy link
Author

@owanturist owanturist Apr 28, 2018

Choose a reason for hiding this comment

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

@rtfeldman I've improved your idea and it looks like that

@owanturist
Copy link
Author

@rtfeldman I've gotten rid from unset and initial in multivalues properties. Could you take a look please?

src/Css.elm Outdated
, vmin : Supported
, vw : Supported
, zero : Supported
, elliptical : Supported
Copy link
Owner

Choose a reason for hiding this comment

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

Hm, looking at how this turned out, I think it's gonna be pretty confusing to people why elliptical is only supported on the final argument. (This is correct, since the spec says you only get one / maximum, but I think it will surprise people who are unfamiliar with this.)

I'm leaning towards saying we shouldn't support the / after all. The docs suggest it's just shorthand for other properties, so it appears there's nothing you can't express a different way if we just didn't support elliptical.

That's sounding simpler at this point...what do you think @owanturist?

Copy link
Author

Choose a reason for hiding this comment

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

@rtfeldman I think so. It's not big deal to support / in this case.

@rtfeldman
Copy link
Owner

Super great! Thank you for all the iteration on this one @owanturist - I'm really happy with the final result! 😻

@rtfeldman rtfeldman merged commit ab1f444 into rtfeldman:phantom-types May 2, 2018
@owanturist owanturist deleted the phantom-border-radius branch May 3, 2018 15:23
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.

2 participants