search

norlab / paperbuzz_viz_test

https://norlab.github.io/paperbuzz_viz_test/index.html
0 stars 3 forks source link

CSS can cause conflicts when embedded #8

Open NateWr opened 6 years ago

NateWr commented 6 years ago

Hi @norlab, I came to this repo from @jalperin's, and noticed something in the implementation that one of my PKP colleagues is building for OJS now.

In this screenshot you can see some CSS code (.title) effecting the styles of non-paperbuzz elements on a page where papebuzz is embedded.

selection_011

Several of the selectors could probably be more tightly scoped to prevent style bleed:

https://github.com/norlab/paperbuzz_viz_test/blob/master/css/paperbuzzviz.css#L1 https://github.com/norlab/paperbuzz_viz_test/blob/master/css/paperbuzzviz.css#L11-L12 https://github.com/norlab/paperbuzz_viz_test/blob/master/css/paperbuzzviz.css#L18 https://github.com/norlab/paperbuzz_viz_test/blob/master/css/paperbuzzviz.css#L23 https://github.com/norlab/paperbuzz_viz_test/blob/master/css/paperbuzzviz.css#L27 https://github.com/norlab/paperbuzz_viz_test/blob/master/css/paperbuzzviz.css#L108 https://github.com/norlab/paperbuzz_viz_test/blob/master/css/paperbuzzviz.css#L112

The use of a prefix for most classes , as you've done (.paperbuzz-*), is usually good enough. But if you have elements which you can't change the class name of, maybe because it's built in a dependency, I'd encourage you to use a wrapper. Something like:

.paperbuzz .bar {
  ...
}

The icomoon font is another potential source of conflicts. I think the risk here is lower but since icomoon is a popular service, it's definitely possible that you'd conflict with another font named icomoon. I believe the icomoon app lets you define the font family name when generating the files, so it might be worth naming it paperbuzz just to be safe.

All just suggestions. Nice work. :+1:

jalperin commented 6 years ago

Should be able to also change the things like "bar" to "paperbuzz-bar". There are no classes that are built into the library, all are coded in the js that builds the viz:

.attr("class", function(d) { return "bar " + viz.z((level == 'day' ...

It requires a few changes throughout, but should be pretty straight forward.

jalperin commented 6 years ago

Hi @NateWr . I've fixed the name spacing issues for the CSS classes. I haven't used icomoon, so I'll leave it to @norlab to see if she can generate the file with paperbuzz in the name space.

Bozana has been working on the OJS plugin to integrate this code into OJS, but I wonder if you might be able to spend a bit of time helping to style things, or even deciding where/how to place the graphs. Now that we have the code written and functioning, it is pretty easy to shuffle around the element markup and to add/change any CSS classes.

NateWr commented 6 years ago

I've been trying not to get more involved in the reader-facing stuff. Have you considered bringing it to the theme team? They've got a designer and developer focused on this stuff and might be able to help.