Skip to content

className #126

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

Closed
wants to merge 1 commit into from
Closed

className #126

wants to merge 1 commit into from

Conversation

Fil
Copy link
Contributor

@Fil Fil commented Feb 16, 2021

Adds a className on all marks, facets and axes; override by setting {className: "my class"} in the options.

Demo: https://observablehq.com/d/44f1024cb20f8a0d

If I then promote all classes to ids, it gives the following structure in Illustrator
Capture d’écran 2021-02-16 à 13 48 40

@Fil Fil requested a review from mbostock February 16, 2021 13:05
@mbostock
Copy link
Member

I think I’d be more comfortable with this if className was optional and had no default value, so you have to decide what names you want to use and there are no implicit classes.

@Fil
Copy link
Contributor Author

Fil commented Feb 16, 2021

I had the following "spec" in mind: "Having a class based on the mark type sound reasonable". But I don't have a strong preference, and the code will be leaner without all the default classes. One argument against default classes is that users will then want them to stay even when they add their own :)

@mbostock
Copy link
Member

It’s always easier to add something than to take something away, too.

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 would support this as an optional className but I don’t think mark types should provide a default className.

@Fil Fil self-assigned this Feb 25, 2021
Fil added a commit that referenced this pull request Mar 18, 2021
@Fil Fil closed this Mar 18, 2021
@Fil Fil deleted the markclass branch March 18, 2021 15:26
@Fil
Copy link
Contributor Author

Fil commented Mar 18, 2021

superseded by #253

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.

2 participants