Skip to content

Keep CSS rules static #179

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

Open
wants to merge 12 commits into
base: main
Choose a base branch
from
Open

Keep CSS rules static #179

wants to merge 12 commits into from

Conversation

quinarygio
Copy link

Please check if the PR fulfills these requirements

  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)
  • A PR or issue has been opened in all impacted repositories (if any)

Does this PR already have an issue describing the problem?

Fixes #154

What kind of change does this PR introduce?

What is the current behavior?

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change or deprecate an API?

  • Yes
  • No

If yes, please check if the following requirements are fulfilled

  • The Breaking Change or Deprecated label has been added
  • The migration steps are described in the following section

What changes might users need to make in their application due to this PR? (migration steps)

Other information:

Signed-off-by: Giovanni Ferrari <[email protected]>
@quinarygio quinarygio linked an issue Mar 18, 2025 that may be closed by this pull request
Signed-off-by: Giovanni Ferrari <[email protected]>
@flo-dup flo-dup mentioned this pull request Mar 19, 2025
5 tasks
@flo-dup flo-dup linked an issue Mar 21, 2025 that may be closed by this pull request
@Tristan-WorkGH
Copy link
Member

Why are the css rules that was in dynamic-css-utils.ts are moved in the demo? Projects that was using this feature will need to copy all theses rules in there project.

Copy link
Contributor

@flo-dup flo-dup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add some doc lines to the constructor to explain the added array parameter?

@flo-dup
Copy link
Contributor

flo-dup commented Mar 21, 2025

Why are the css rules that was in dynamic-css-utils.ts are moved in the demo? Projects that was using this feature will need to copy all theses rules in there project.

You mean to provide a default css file which corresponds to the previous rules and that could be easily imported by the user? Then it would only makes sense for the default array. But yes I agree we should add such a file, it's still handy to have a ready-to-use feature.

.nad-vl120to180-0, .nad-vl120to180-1, .nad-vl120to180-2, .nad-vl120to180-3, .nad-vl120to180-4, .nad-vl120to180-5, .nad-vl120to180-6, .nad-vl120to180-7, .nad-vl120to180-8, .nad-vl120to180-line,
.nad-vl180to300-0, .nad-vl180to300-1, .nad-vl180to300-2, .nad-vl180to300-3, .nad-vl180to300-4, .nad-vl180to300-5, .nad-vl180to300-6, .nad-vl180to300-7, .nad-vl180to300-8, .nad-vl180to300-line
) {
display: block
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you remove all the display: block rules?

Comment on lines 369 to 376
.nad-zoom-1000 :is(
.nad-branch-edges .nad-edge-path,
.nad-3wt-edges .nad-edge-path,
.nad-branch-edges .nad-winding,
.nad-3wt-nodes .nad-winding,
.nad-vl-nodes circle.nad-unknown-busnode) {
stroke-width: 0.25%
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this rules are common for most of the zooms, so you could have it as default, in order to see better what's changing

Comment on lines 415 to 417
.nad-zoom-2500 .nad-disconnected .nad-edge-path {
stroke-dasharray: 0.5% 0.5%
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for this stroke

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please keep the rules available to projects currently using them, not just in the demo.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @flo-dup suggest, either as default that can be override or at least as importable rules or css.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the first suggestion, in fact I don't think we should put any default CSS in the component. That would mean injecting some CSS? Then how would a webpage with two components work, there would be conflicts with the two CSS?

Instead as you said we could give one or several CSS examples. But to me it's not clear how we could include a CSS file in the release.

Another simpler way to do this would be to leave the examples in the demo, and just add in the next migration guide the CSS which corresponds to the previous default DEFAULT_DYNAMIC_CSS_RULES: CSS_RULE[]. Indeed it's just a matter of migration, isn't it?

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.

Keep CSS rules static css in svg style bleed into the global css
3 participants