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

Color Comments #70

Merged
merged 2 commits into from
Feb 19, 2016
Merged

Color Comments #70

merged 2 commits into from
Feb 19, 2016

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Feb 18, 2016

I added my comments below the TODO's.

The color('white') constructor I implemented is lowercase, similar to rgb(), hsl(), etc. but I can see why it could be argued to be uppercase. I changed all occurrences here to lowercase, but if I should change to implementation to uppercase, let me know.

I added my comments below the TODO's.

The `color('white')` constructor I implemented is lowercase, similar to `rgb()`, `hsl()`, etc. but I can see why it could be argued to be uppercase. I changes all occurrences here to lowercase, but if I should change to implementation to uppercase, let me know.
@ggetz ggetz mentioned this pull request Feb 18, 2016
85 tasks
@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 18, 2016

OK, the convention for JavaScript constructor functions is Pascal case, as is the convention for JavaScript types, e.g., Number(1).

@ggetz
Copy link
Contributor Author

ggetz commented Feb 18, 2016

So in that case should we also make rgb(), etc. Pascal case?

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 18, 2016

I meant to just open PRs into Cesium for things, not have the discussion in the .md file. Moving the comments here:

I changed all occurrences here to lowercase, but if I should change to implementation to uppercase, let me know

Make it uppercase throughout.

If we want to go with Cesium behavior, an empty color() produces white.

We should follow the CSS3 Colors spec. See what it says.

"color" : "rgb(255, 0, 0, (${Visible} ? 255 : 0))"

we should not have the %. Also % do not parse wil jsep, which would make the % difficult to implement.

We would just pre-process them, but I agree I don't like them either and they don't fit into a strict grammar.

TODO: should color have an optional second argument for alpha, e.g., Color('red', 0.5)? I think so.

// while that is not normal css bevhavior, I would say that is easy to implement and useful, so yes.

CSS doesn't have a Color type in the same sense we do so this is still "CSS behavior" in that our Color type takes a CSS string and separate alpha.

TODO: is keyword case sensitive?

// According to W3, they are not case sensative

OK, just add case-insensitive before the first type our spec says "keyword"

TODO: rgb, hsl, rgba, and hsla should require all their arguments (assuming that is true in CSS).

// Yes, currently I have a todo in the code to check for this.

Please do this soon so we can keep the spec and implementation in sync.

So in that case should we also make rgb(), etc. Pascal case?

No, think of them as regular functions that return a Color object.

@ggetz
Copy link
Contributor Author

ggetz commented Feb 18, 2016

I meant to just open PRs into Cesium for things, not have the discussion in the .md file.

Sorry, my confusion.

@ggetz
Copy link
Contributor Author

ggetz commented Feb 18, 2016

rgb, hsl, rgba, and hsla should require all their arguments
Please do this soon so we can keep the spec and implementation in sync.

OK

@ggetz
Copy link
Contributor Author

ggetz commented Feb 18, 2016

We should follow the CSS3 Colors spec. See what it says.

The spec does not specifically mention empty values.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 18, 2016

OK, white is fine. Please update our spec in this branch.

@ggetz
Copy link
Contributor Author

ggetz commented Feb 18, 2016

Updated.

@pjcozzi
Copy link
Contributor

pjcozzi commented Feb 19, 2016

Nicely done!

pjcozzi added a commit that referenced this pull request Feb 19, 2016
@pjcozzi pjcozzi merged commit 6a988c6 into style Feb 19, 2016
@pjcozzi pjcozzi deleted the ggetz-patch-1 branch February 19, 2016 00:16
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