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

Add @jupyterlab/ui-components package #5538

Merged
merged 25 commits into from Jan 10, 2019

Conversation

@gnestor
Copy link
Contributor

@gnestor gnestor commented Oct 26, 2018

This work is a first shot at #5170 and continuation of #4234

This package will provide React components that other lab extensions can consume to easily create UIs within lab.

Initially, this PR:

  • Imports blueprint components and exports them with a CSS class name applied
  • Imports the blueprint CSS and a custom CSS file that will allow us to define styles for individual components (e.g. jp-Button). We don't have to define styles from scratch since the blueprint CSS is applied, we just need to override styles in our own classes.

To contribute to this PR:

  • Add my remote/fork: git remote add gnestor https://github.com/gnestor/jupyterlab.git
  • Fetch my remote: git fetch gnestor
  • Checkout this branch: git checkout gnestor/ui-components
  • Install dependencies: jlpm
  • Watch the source: jlpm watch
  • Launch lab in dev and watch mode: jupyter lab --dev --watch
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Oct 26, 2018

Update:

  • I have just exported a Button and Select component to demonstrate how it could be done
  • I have demonstrated how the Button component could be used in the @jupyterlab/extensionmanager package (because it already uses React/vdom)

To do:

  • Demonstrate how to use it in a package that doesn't use vdom (e.g. celltools)

Questions:

  • Should we use CSS or Typestyle? I don't fully understand the benefits of Typestyle. One issue I foresee is typo discrepancies between class names used in Typescript (e.g. jp-Button) and in CSS (e.g. jpButton). There is no way to catch this reference error as long as we're using CSS but I assume that Typestyle could catch that.

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Oct 26, 2018

I'm also still trying to wrap my head around Typestyle. It is used extensively in the new status bar. One consequence of using is that, since it is in JS, it introduces more dependencies among different packages that want to use its styles. Of course, components are implicitly dependent on plain CSS styles, so maybe it is better to be more strict, as Typestyle requires. I'm not sure at the moment, but there area certainly some tradeoffs there.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Oct 26, 2018

Ok, so 2 downsides to Typestyle is a) consumers of this ui-components package will need to a) depend on Typestyle and b) learn how to use it (vs. CSS which is standard and probably well-understood by anyone creating a lab extension).

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Oct 26, 2018

@jennalandy points out one upside of using Typestyle:

...Typestyle makes it really easy to make styling a function of props rather than writing different classes for all possible variations.

export const JLButton = (props: IJLButtonProps) => 
  <Button {...props} className={JLButtonStyle(props.type, props.disabled)}/>

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Oct 26, 2018

I should note that typestyle also has a pure CSS mode that gives you the benefit of being able to specify the classname and selector explicitly, while still giving you validated and type-checked CSS:

https://typestyle.github.io/#/raw

I think for situations like this, that is probably a better usage of typestyle (than having it auto-generate cryptic class names automatically). This may also be relevant in the status bar work as well.

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Oct 26, 2018

The question of should we use typestyle or something else may need its own issue, but the things that were causing us to go that way were:

  • Being forced to invent CSS classnames always is really painful. CSS classnames also have a way of becoming part of a public API, when they weren't intended to.
  • Maintaining our CSS without nested rules is really painful. Less and scss also solve this issue and would be an alternative to typestyle. Because we are using native CSS variables though, there are fewer reasons to use less/scss.

Thoughts?

@ian-r-rose
Copy link
Member

@ian-r-rose ian-r-rose commented Oct 27, 2018

@ellisonbg
Copy link
Contributor

@ellisonbg ellisonbg commented Oct 27, 2018

good point @ian-r-rose

It is definitely the case that sometimes we do want CSS variables to be public APIs. Although I tend to think of functional ones (used for keyboard shortcuts, menus, etc.) should be distinct from those that control visual style (which I would say shouldn't be public in most cases).

@gnestor gnestor force-pushed the ui-components branch 4 times, most recently from 8fc8521 to 6f1be0a Nov 5, 2018
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Nov 6, 2018

This is ready to review @ellisonbg @ian-r-rose @saulshanabrook

I'll take a look at the failed tests now...

@saulshanabrook saulshanabrook self-requested a review Nov 6, 2018
select.value = mType;
this._changeGuard = false;
return (
<HTMLSelect
Copy link
Member

@saulshanabrook saulshanabrook Nov 6, 2018

This is much more tidy!

Copy link
Member

@saulshanabrook saulshanabrook left a comment

Thanks for this!

I added a few comments of little things to clean up.

How does the CSS generated with styledoc end up in the build? Is it included in the JS file or does it end up compiled into the CSS file by webpack?

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Nov 6, 2018

@saulshanabrook By styledoc I assume you mean typestyle? style({ ... }) calls in style.js return a unique class name. Behind the scenes, it's deduping classes and some other helpful stuff and then it adds a stylesheet to the document with the styles for those class names.

Before we merge this, I'd like to add one more commit (in addition to the clean up stuff that Saul suggested) that demonstrates how this PR would work with plain old CSS. We can discuss the pros/cons of each and decide which one to go with moving forward.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Nov 7, 2018

By styledoc I assume you mean typestyle?
Yes :)

Behind the scenes, it's deduping classes and some other helpful stuff and then it adds a stylesheet to the document with the styles for those class names.

So then I assume it ends up compiling to just a JS file, the CSS doesn't get extracted, right?

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Nov 7, 2018

We end up with typedstyled button:

image

and this appended to the end of <head>:

image

If I'm not mistaken, all of the typestyle styles are consolidated into one stylesheet.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Nov 7, 2018

In the meeting we talked about switching to styledoc, but wanted to clarify that we have to preserve some CSS classes for semantic use, even if we don't use them styling: #5600

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Nov 7, 2018

Also opened #5601 to make see if we can drop the components in blueprint we aren't useing from the bundle

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Nov 7, 2018

It looks like these components add about 2.6mb to our bundle size: #5170 (comment)

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Jan 10, 2019

@ellisonbg @ian-r-rose Travis build is passing but the other two aren't. It looks like Azure Pipelines is failing on most open PRs but I see a few where it's passing 🤔 The Appveyor build is failing due to a cancelled promise which I was able to resolve in Travis by just restarting the job, but I don't see an option to do that in Appveyor.

@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jan 10, 2019

@gnestor Azure is failing b/c of the doc error that @ian-r-rose fixed yesterday I believe. This is good to merge in from my perspective.

@saulshanabrook saulshanabrook added this to the 1.0 milestone Jan 10, 2019
@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Jan 10, 2019

Thanks @saulshanabrook. You have my support to merge 👍

@saulshanabrook saulshanabrook merged commit 3d88337 into jupyterlab:master Jan 10, 2019
1 of 3 checks passed
@gnestor gnestor deleted the ui-components branch Jan 10, 2019
@saulshanabrook
Copy link
Member

@saulshanabrook saulshanabrook commented Jan 10, 2019

Thanks @gnestor for keeping on this! Glad to finally have it in :D

@echarles
Copy link
Contributor

@echarles echarles commented Jan 10, 2019

Congrats to all.

@echarles
Copy link
Contributor

@echarles echarles commented Jan 10, 2019

You've been super resilient on this one @gnestor. Congrats and looking forward to use this new module.

@gnestor
Copy link
Contributor Author

@gnestor gnestor commented Jan 10, 2019

Thank you @echarles! It sure feels to get have it merged. Now time to iterate!

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.

None yet

6 participants