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

Load modeling styles #3031

Merged
merged 2 commits into from Jul 18, 2022
Merged

Load modeling styles #3031

merged 2 commits into from Jul 18, 2022

Conversation

pinussilvestrus
Copy link
Contributor

@pinussilvestrus pinussilvestrus commented Jul 15, 2022

Related to https://github.com/bpmn-io/internal-docs/issues/590
Closes #3030

  • Remove patch CSS fixes
  • Load modeling library styles globally (in an ordered manner)

Summary

Find some more context in https://github.com/bpmn-io/internal-docs/issues/590#issuecomment-1181765569 and https://github.com/bpmn-io/internal-docs/issues/590#issuecomment-1185305654.

Instead of loading the camunda-bpmn|dmn-js modeling styles on the editor component level, and causing duplicated style definitions in head, it now loads everything in a central place. This has the advantage that we have full control of when and in which order the styles are loaded.

Note that we do not use the "bundled" modeler styles from the packages, but the individual components to prevent duplicates. This only works under the assumption that we anyway want only one version of library styles in BPMN and DMN.

Artifacts

Todo

  • Build artifacts (is running...).
  • Gather feedback from the team
  • Consider test coverage?

@pinussilvestrus pinussilvestrus requested a review from a team July 15, 2022 08:42
@bpmn-io-tasks bpmn-io-tasks bot added the in progress Currently worked on label Jul 15, 2022
@pinussilvestrus pinussilvestrus requested review from a team, Skaiir, barmac, philippfromme and marstamm and removed request for a team July 18, 2022 07:38
@pinussilvestrus pinussilvestrus marked this pull request as ready for review July 18, 2022 07:39
@bpmn-io-tasks bpmn-io-tasks bot added needs review Review pending and removed in progress Currently worked on labels Jul 18, 2022
@barmac
Copy link
Contributor

barmac commented Jul 18, 2022

I am wondering if we can automate this so that we don't need to update _modeling.less when a new stylesheet is added to any of camunda-xxxx-js packages. We know upfront that the file names like bpmn-js.css or diagram-js.css should be loaded only once, so perhaps we could filter them out with the webpack configuration. Thus, we could keep camunda-xxxx-js styles as a blackbox. WDYT?

@barmac
Copy link
Contributor

barmac commented Jul 18, 2022

BTW I like the idea of preloading styles in one place instead of lazy-loading with each of the editor tabs.

@pinussilvestrus
Copy link
Contributor Author

I am wondering if we can automate this so that we don't need to update _modeling.less when a new stylesheet is added to any of camunda-xxxx-js packages. We know upfront that the file names like bpmn-js.css or diagram-js.css should be loaded only once, so perhaps we could filter them out with the webpack configuration. Thus, we could keep camunda-xxxx-js styles as a blackbox. WDYT?

I like this idea; let's try it out. It should be possible around the style-loader#insert configuration. Our used style-loader version is fairly outdated, so let's see how easy it is to make it configurable.

@pinussilvestrus
Copy link
Contributor Author

pinussilvestrus commented Jul 18, 2022

Seems like it's a highly needed, but not yet supported feature to identify the generated <style> tags, e.g. to filter duplicates (cf. webpack-contrib/style-loader#216 (comment)).

There might be a workaround though.

Copy link
Contributor

@barmac barmac left a comment

Choose a reason for hiding this comment

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

IMO the workaround looks more complicated than our workaround. Let's merge this as is.

@pinussilvestrus
Copy link
Contributor Author

There might be other options using postcss: webpack-contrib/css-loader#1233. But I agree; let's keep things simple for now. If we see the necessity, let's get back to 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.

None yet

2 participants