[fix] Remove global CSS rules to avoid conflicts #303 Refactored CSS to prevent global styling conflicts with downstream projects. Fixes #303 #317
Conversation
Unnati-Gupta24
changed the title
Unnati-Gupta24
changed the title
| .netjsongraph-container { | ||
| position: absolute; | ||
| top: 0; | ||
| left: 0; | ||
| width: 100%; | ||
| height: 100%; | ||
| } |
There was a problem hiding this comment.
Do we need to duplicate these rules in all examples? You have already defined this in src/css/netjsongraph.css
There was a problem hiding this comment.
Thanks for pointing that out, @pandafy. You're absolutely right—the CSS rules are already defined in src/css/netjsongraph.css, so duplicating them in the example_templates folder is unnecessary. I've removed the duplicate code and updated the PR accordingly.
However it is taking sometime to reflect the changes the update shall be visible soon.
Thanks again!
There was a problem hiding this comment.
I've changed the code accordingly.
Kindly check it. @pandafy
Unnati-Gupta24
changed the base branch from
master
to
dependabot/npm_and_yarn/babel/preset-env-7.26.7
Unnati-Gupta24
changed the base branch from
dependabot/npm_and_yarn/babel/preset-env-7.26.7
to
0.2
There was a problem hiding this comment.
I am not convinced about these changes, I leave some comments here but I think we'll need to double check this with @pandafy.
| @@ -14,6 +14,13 @@ | |||
| <link href="../lib/css/netjsongraph-theme.css" rel="stylesheet" /> | |||
| <link href="../lib/css/netjsongraph.css" rel="stylesheet" /> | |||
| <style type="text/css"> | |||
| html, body { | |||
There was a problem hiding this comment.
Let's avoid repeating this in each example. We can make a CSS file and store it in public/asstes/css/, should be doable.
There was a problem hiding this comment.
ya I'm making a separate file for example specific stylings
| <div class="netjsongraph-container"> | ||
| <div id="graphChartContainer"></div> | ||
| </div> |
There was a problem hiding this comment.
Can we shorten this a bit, eg?
| <div class="netjsongraph-container"> | |
| <div id="graphChartContainer"></div> | |
| </div> | |
| <div id="njg"> | |
| </div> |
The njg acronym stands for netjsongraph, it's already used elsewhere in the code.
| font-size: 14px; | ||
| overflow: hidden; | ||
| } | ||
|
|
||
| h3 { |
There was a problem hiding this comment.
I also see this and a @font-face directive which don't look right here.
I think the @font-face can be moved to the CSS file dedicated to the examples.
Regarding the h3, can we precede it with some other class?
There was a problem hiding this comment.
Adding a few more comments:
I am not sure if the library does this, but the right thing to do would be to inject an HTML element with a predefined CSS class, so that we prefix everything in the CSS with that class. We can't style main elements like body, html, h3 in a css that is meant to be included anywhere, nor we can load fonts.
Any styling meant for the examples shall be moved in the examples CSS file.
What do you think @pandafy?
I agree with @nemesifier |
|
Ok thank you for the suggestions. I'm working on them. |
Checklist
Reference to Existing Issue
Closes #303 .
Description of Changes
The initial issue was that CSS styles were being applied globally using the * selector, which can cause conflicts in larger projects. Here's how I fixed it:
Created a scoped container structure:
Added .netjsongraph-container as main wrapper
Added #graphChartContainer for the graph content
This contains styles to just these elements instead of affecting entire page
Fixed height/positioning issue:
Set proper html/body height to 100%
Made container absolute positioned with full dimensions
This ensures graph fills available space correctly
Updated JavaScript:
This approach prevents style leaks while maintaining the original functionality.
@pandafy @nemesifier please review this pull request.
I have made appropriate changes to fix the issue.