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

CSS import refactor #6395

Merged
merged 14 commits into from Jun 10, 2019
Merged

CSS import refactor #6395

merged 14 commits into from Jun 10, 2019

Conversation

@vidartf
Copy link
Member

@vidartf vidartf commented May 23, 2019

References

Partially addresses #6392.

Fixes #6390.

Code changes

WIP

User-facing changes

None.

Backwards-incompatible changes

Dependent packages will now have to import the style of the packages they use. This change will not cause a compile/build error. When viewing a page, a failure to include the styles will most likely be a glaring issue, but it could also manifest as subtle behavior difference.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented May 23, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@blink1073
Copy link
Member

@blink1073 blink1073 commented May 30, 2019

New Plan, General Pattern:

  • modules have an index.css that imports the index.css of direct dependencies
  • modules have a base.css that only imports local css to support overrides
  • explicit paths in the index.css in the extensions until the style-loader is released
  • add the style tag to all packages that have styles that points to index.css
  • do not import css in TS in packages/
  • we template the full path in dev_mode/index.js and require statically based on installed extensions that have a style field in their package.json
  • check if we can make any smart changes to integrity scripts to help maintain/encourage this state

Then, after CSS loader release, we update the css imports and dev_mode to use the root import. We update the cookie cutters at this time as well.

@vidartf
Copy link
Member Author

@vidartf vidartf commented May 30, 2019

Clarification point: Extensions should not import the styles of the default implementor of a token they require. The dev_mode script will handle that. This is what is meant by "driect dependency" above.

@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 4, 2019

@blink1073 I think I've done most of the work needed here now. There are some outstanding things though:

  • Add stuff to metapackage automatically.
  • Decide what to do about packages in services/examples and test packages.
  • Review :)

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 4, 2019

  • Do you mean add stuff to dev_mode/index.js automatically?
  • I think it is fair for those to import css directly since they aren't mean to be extended.

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 4, 2019

Also, needs a rebase 😉

@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 5, 2019

Rebase completed. I'm not exactly sure what you were going to do, but the new integrity errors for metapackage needs to be addressed somehow also :)

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 6, 2019

Note to self: update template package as well

@vidartf
Copy link
Member Author

@vidartf vidartf commented Jun 6, 2019

Note: Rebased, so take care to pull --rebase !

@blink1073 blink1073 added this to the 1.0 milestone Jun 6, 2019
@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 10, 2019

All that's left here is updating the examples and convincing myself that core-mode will work as intended (we can't test for sure until after the next release).

@blink1073 blink1073 changed the title [WIP] CSS import refactor CSS import refactor Jun 10, 2019
@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 10, 2019

This is ready (pending a good build)!

@@ -0,0 +1,35 @@
/* This is a generated file of CSS imports */
Copy link
Member Author

@vidartf vidartf Jun 10, 2019

It would be nice if this said which file generated this one.

Copy link
Member

@blink1073 blink1073 Jun 10, 2019

I'll add it to my follow up list. I think we need to move ahead on this PR

Copy link
Member Author

@vidartf vidartf Jun 11, 2019

👍

@import './splash.css';
@import './base.css';

@import url('~@jupyterlab/apputils/style/index.css');
Copy link
Contributor

@jasongrout jasongrout Jun 10, 2019

In general, I think the dependencies should be imported first, and then the base.css, so that the extension has a chance to override things in the dependency css if needed.

Copy link
Member

@blink1073 blink1073 Jun 10, 2019

Part of the follow up task is to formalise this during the rc phase

Copy link
Member

@blink1073 blink1073 Jun 10, 2019

And automate it

Copy link
Contributor

@jasongrout jasongrout Jun 10, 2019

sounds good.

@@ -16,8 +16,12 @@
"lib/**/*.{d.ts,eot,gif,html,jpg,js,js.map,json,png,svg,woff2,ttf}",
"style/**/*.{css,eot,gif,html,jpg,json,png,svg,woff2,ttf}"
],
"sideEffects": [
"style/*"
Copy link
Contributor

@jasongrout jasongrout Jun 10, 2019

"style/**/*" for consistency?

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 10, 2019

I won't be logging back on today, so feel free to take over or we can wait until tomorrow

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 10, 2019

This looks good from my perspective, with some follow-ups in a different PR for consistency and automation in a new PR. I think this has a high chance for conflicts, so would rather get this in as-is.

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 10, 2019

Thanks for working on this @vidartf and @blink1073!

afshin
afshin approved these changes Jun 10, 2019
Copy link
Member

@afshin afshin left a comment

This also looks good to me. I think it deserves a little bit of show & tell on Wednesday at our meeting, for the team to be on the same page.

@jasongrout jasongrout merged commit f5d76fd into jupyterlab:master Jun 10, 2019
9 checks passed
@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Jun 10, 2019

All green now too!

Great work. Thanks again.

@blink1073 blink1073 mentioned this pull request Jun 10, 2019
1 task
@vidartf vidartf deleted the css branch Jun 11, 2019
@lock
Copy link

@lock lock bot commented Aug 6, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related discussion.

@lock lock bot locked as resolved and limited conversation to collaborators Aug 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants