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

Add className option for marks #1098

Merged
merged 9 commits into from
Jul 3, 2024

Conversation

RLesser
Copy link
Contributor

@RLesser RLesser commented Oct 22, 2022

see #1002 (and related discussions).

fixes #1002

I believe with this implementation className will be an option on all marks.

Open questions

  • Other capabilities that should be tested?
  • Should the two maybeClassName functions be combined in a cleaner way?

@RLesser
Copy link
Contributor Author

RLesser commented Oct 22, 2022

Looks like the jobs are failing due to a checkout issue? lmk if I need to construct my pull request differently.

@Fil
Copy link
Contributor

Fil commented Oct 27, 2022

Do the tests pass locally?

Fil
Fil previously requested changes Oct 27, 2022
Copy link
Contributor

@Fil Fil left a 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.

@espinielli
Copy link
Contributor

This seems interesting.
My use case would be for a Plot.image marks where I could define a border via CSS to the their class of using something like:

.my_image {
    outline: 6px solid white;
}

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.
@RLesser
Copy link
Contributor Author

RLesser commented Dec 28, 2022

@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.

@RLesser
Copy link
Contributor Author

RLesser commented Jan 16, 2023

@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 Error: A branch or tag with the name 'add-className-option-for-marks' could not be found for that check.

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:
Screen Shot 2023-01-16 at 1 42 01 PM
Screen Shot 2023-01-16 at 1 42 20 PM

Do you think it might be a permission issue? Is there some other way I should be creating the branch or pull request?

@mbostock
Copy link
Member

mbostock commented Jan 16, 2023

The Prettier issues are almost certainly a problem with our GitHub workflow, likely these lines:

with:
# Make sure the actual branch is checked out when running on pull requests
ref: ${{ github.head_ref }}
# This is important to fetch the changes to the previous commit
fetch-depth: 0

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:

echo ::add-matcher::.github/eslint.json
yarn run eslint . --format=compact

We should investigate consolidating the Prettier checks into the main workflow. #1227

@RLesser RLesser requested a review from Fil January 18, 2023 16:42
@jcolot
Copy link

jcolot commented Jun 27, 2024

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

@RLesser
Copy link
Contributor Author

RLesser commented Jul 1, 2024

@Fil do you view this as still worth getting on? If so I can clean up the conflicts and get it ready for review.

@Fil
Copy link
Contributor

Fil commented Jul 1, 2024

Yes, I still think it would be a useful feature. Thanks!

@mbostock mbostock self-assigned this Jul 3, 2024
Copy link
Member

@mbostock mbostock left a 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. 👍

@mbostock mbostock dismissed Fil’s stale review July 3, 2024 23:34

Redesigned maybeClassName

@mbostock mbostock changed the title Add class name option for marks Add className option for marks Jul 3, 2024
@mbostock mbostock merged commit 8e52154 into observablehq:main Jul 3, 2024
1 check passed
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.

className option for marks
5 participants