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

Use CSS Modules in Graph System #12948

Merged
merged 9 commits into from
Sep 7, 2022
Merged

Use CSS Modules in Graph System #12948

merged 9 commits into from
Sep 7, 2022

Conversation

carolhmj
Copy link
Contributor

@carolhmj carolhmj commented Sep 5, 2022

This is part of the work to make the graph system more reusable.

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@azure-pipelines
Copy link

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

Looks good to me aside of the tiny issue :-) but I ll let @RaananW do the final review

packages/dev/buildTools/src/webpackTools.ts Outdated Show resolved Hide resolved
@azure-pipelines
Copy link

Copy link
Member

@RaananW RaananW left a comment

Choose a reason for hiding this comment

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

Thank you so much for that :-)

I just want to be sure that it doesn't influence the existing tools before merging. Does it change anything in the node editor/gui editor? As it was previously globally applied, it is possible that those classes that were removed are used in one of the tools.

Otherwise - approved, great work

@RaananW RaananW requested a review from sebavan September 6, 2022 15:35
@carolhmj
Copy link
Contributor Author

carolhmj commented Sep 6, 2022

Thank you so much for that :-)

I just want to be sure that it doesn't influence the existing tools before merging. Does it change anything in the node editor/gui editor? As it was previously globally applied, it is possible that those classes that were removed are used in one of the tools.

Otherwise - approved, great work

Great point! I was already testing with the node editor, and tested with the GUI editor now and everything seems to be on the right place 😄

Copy link
Member

@sebavan sebavan left a comment

Choose a reason for hiding this comment

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

Looks all good to me, @RaananW can we merge it ?

@RaananW
Copy link
Member

RaananW commented Sep 7, 2022

I'll wait with the merge until azure is stable, but yes, we can merge

@sebavan sebavan merged commit d8e38d9 into BabylonJS:master Sep 7, 2022
RaananW pushed a commit that referenced this pull request Dec 9, 2022
Use CSS Modules in Graph System

Former-commit-id: a0a47aadd38d44a76a31a909d7e1ac43776d6cc1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants