typestyle / csx

Utility functions for TypeStyle
https://typestyle.github.io
MIT License
102 stars 14 forks source link

`rgb().toString()` should format the string the same way `window.getComputedStyle($0).color` returns the value #84

Closed basarat closed 5 years ago

basarat commented 5 years ago

Current:

rgb(0, 0, 0).toString(); // rgb(0,0,0)

Desired:

rgb(0, 0, 0).toString(); // rgb(0, 0, 0)

getComputedStyle see in browser

Tested on chrome: image

Motivation

To help with unit testing:

Requested on twitter : https://twitter.com/cromas/status/1149480690796224513 :rose:

craigkovatch commented 5 years ago

This was my request, thank you! Our designers communicate colors in hex codes, but when looking at rendered output using getComputedStyle the browser returns human-formatted RGB codes like rgb(0, 0, 0). I want to be able to keep our color consts in code in hex -- for easy communication between devs and designers -- but compare them against the rendered (i.e. formatted) output in unit tests. CSX would make this comparison very easy if this formatting request is adopted :)

craigkovatch commented 5 years ago

Point of clarification though -- this isn't just a request that rgb().toString() formatting changes, but also/especially any color converted into rgb or rgba representation.

notoriousb1t commented 5 years ago

On the surface, I don't have an issue with this change. It seems reasonable considering the spaces really only would impact css over the wire when using SSR. My only concern is that it has the potential to break a lot of unit tests in the wild so I am not 100% sure it is wise to make a sweeping change.

Some potential solutions:

  1. Change the signature of toString() to have an optional parameter pretty: boolean which adds spaces.

  2. Add another method toPrettyString() (or something, we can bikeshed) to Color is intended to match the Computed style.

Thoughts?

craigkovatch commented 5 years ago

I think it's ironic that we're worried about breaking unit tests, given the purpose of this request ;)

Would probably want to name it to indicate the intended function, i.e. not just to be pretty but to match the browser computed representation of it. Important question there: is that standard, or is it left up to the user-agent? Doesn't do much good to match Chrome's formatting if Firefox has a different heuristic, etc.

I think a named method is clearer than a single boolean param (without something akin to named parameters in C#, random booleans are a bit opaque)

notoriousb1t commented 5 years ago

The format being suggested is part of the CSSOM spec: https://drafts.csswg.org/cssom/#serializing-css-values, so that makes me think the right answer is to correct our formatting and do a major version bump. I'll fix this and put in a PR.

craigkovatch commented 5 years ago

Sounds perfect, thank you!

notoriousb1t commented 5 years ago

Ok, I added this to the head branch. I am going to do address a few other tickets and publish this as a major version.

notoriousb1t commented 5 years ago

This was published in https://github.com/typestyle/csx/releases/tag/v10.0.0. Please take a look and let me know if that works for you.