top-level className #582
Merged
top-level className #582
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mbostock
reviewed
Oct 25, 2021
src/style.js
Outdated
| } | ||
|
|
||
| export function validClassName(str) { | ||
| return typeof str === "string" && |
There was a problem hiding this comment.
I would expect string coercion here instead of requiring the string type exactly.
I’m curious why we’re validating the class name? Is it just to avoid people using it to inject arbitrary styles into our style sheet? That seems reasonable if so.
There was a problem hiding this comment.
Yes, I mainly wanted to avoid multiple classes (space-separated), which would break things. Then realized we didn't want curly brackets for the reason you're saying. Validating against the spec seemed like a good idea to avoid any shenanigans—maybe overkill tho. We can remove type check.
Base automatically changed from
fil/inline-styles-test
to
mbostock/inline-styles
October 26, 2021 02:09
mbostock
added a commit
that referenced
this pull request
Oct 26, 2021
* inline styles; tabular nums * inline styles test (#581) * keep inline stylesheet * inline stylesheets * top-level className (#582) * keep inline stylesheet * inline stylesheets * top-level className * coerce className to string Co-authored-by: Mike Bostock <[email protected]> Co-authored-by: Philippe Rivière <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Se discussion in #579 (review)