-
Notifications
You must be signed in to change notification settings - Fork 185
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
Add className option for marks #1098
Add className option for marks #1098
Conversation
Looks like the jobs are failing due to a checkout issue? lmk if I need to construct my pull request differently. |
Do the tests pass locally? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would change the logic a bit, instead of introducing a new function maybeClassNameOptional (which is a bit redundant), I would instead add a second parameter to maybeClassName(name, provide)
and have plot, ramp, and swatches, call it with provide = true.
This seems interesting.
Looking forward to some progress ;-) |
Removed the maybeClassNameOptional function, and fixed the existing maybeClassName function calls to take in a "provide" boolean. Added the test output file as well.
@Fil updated with new logic. Tests pass, but something about the prettier action is failing due to "A branch or tag with the name 'add-className-option-for-marks' could not be found". Not sure why that is. |
@Fil, I think there might be an issue with users who aren't @Fil or @mbostock getting their pull requests past the prettier check. I'm getting a I looked back thru past runs of that action and found that the two most recent times that other's have put up pull requests similar things have happened: Do you think it might be a permission issue? Is there some other way I should be creating the branch or pull request? |
The Prettier issues are almost certainly a problem with our GitHub workflow, likely these lines: plot/.github/workflows/prettier.yml Lines 14 to 18 in a946fd9
I’m not sure why we have a separate workflow for Prettier given that ES Lint is already run as part of the main workflow here: plot/.github/workflows/node.js.yml Lines 27 to 28 in a946fd9
We should investigate consolidating the Prettier checks into the main workflow. #1227 |
In the meantime, an easy way to do this is by using the new render option, an example here: https://observablehq.com/@jcolot/observable-plot-adding-classes-to-marks |
@Fil do you view this as still worth getting on? If so I can clean up the conflicts and get it ready for review. |
Yes, I still think it would be a useful feature. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m going to tweak how this is implemented, but the functionality looks right. 👍
see #1002 (and related discussions).
fixes #1002
I believe with this implementation className will be an option on all marks.
Open questions