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

proofreading from @gabalafou #12

Merged
merged 1 commit into from
Feb 26, 2022
Merged

proofreading from @gabalafou #12

merged 1 commit into from
Feb 26, 2022

Conversation

gabalafou
Copy link
Contributor

I made a bunch of edits while reading the doc. I'm not sure I got all of them 100% right. :)

Copy link
Contributor Author

@gabalafou gabalafou left a comment

Choose a reason for hiding this comment

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

This represents a lot of work, so congratulations for getting to this stage!

I just have a few high-level considerations.

Would it be useful to state your intentions for next steps assuming this proposal is accepted? I know there is a section titled "Future possibilities", so let me be more specific. When I chatted with @isabela-pf about this proposal, I realized that I wasn't 100% sure if the intention was to provide a toolkit for future external extensions and future (non-existent) UI work, or if the toolkit would be a foundation for the immediate next step of converting existing UI to use the toolkit. My understanding now is that the first next step if this proposal is accepted would be using it to start converting existing JupyterLab UI.

The only other thing that really stood out for me was that I was expecting to see some discussion around performance or bundle size. Do we have some idea of how this work would impact the size or performance of the app?

That's all for now. Again, awesome work!

jep/README.md Show resolved Hide resolved
jep/README.md Show resolved Hide resolved
jep/README.md Show resolved Hide resolved
jep/README.md Show resolved Hide resolved
Copy link
Member

@echarles echarles left a comment

Choose a reason for hiding this comment

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

Thx a lot @gabalafou

@echarles echarles merged commit 5951d98 into datalayer:jep Feb 26, 2022
@echarles
Copy link
Member

echarles commented Feb 26, 2022

This represents a lot of work, so congratulations for getting to this stage!

Thx, and thanks a lot for your implications and contributions in this. This is a team work, @fcollonval and @jtpio have contributed to this document and @fcollonval is rocking the boat with the development track of the components.

Would it be useful to state your intentions for next steps assuming this proposal is accepted? I know there is a section titled "Future possibilities", so let me be more specific. When I chatted with @isabela-pf about this proposal, I realized that I wasn't 100% sure if the intention was to provide a toolkit for future external extensions and future (non-existent) UI work, or if the toolkit would be a foundation for the immediate next step of converting existing UI to use the toolkit. My understanding now is that the first next step if this proposal is accepted would be using it to start converting existing JupyterLab UI.

@gabalafou @isabela-pf I guess we want in the future any core frontend and any external extensions to rely. This will be a long road and the first step I hope would be to use it in JupyterLab core. Once the JEP in, this will need to be discussed at JupyterLab meeting, but as this is reviewed by a bunch of core JupyterLab developers, it should be easier to discuss.

The only other thing that really stood out for me was that I was expecting to see some discussion around performance or bundle size. Do we have some idea of how this work would impact the size or performance of the app?

I don't think we have, that will be important to review.

@fcollonval
Copy link
Member

Thanks a lot @gabalafou for the review

Would it be useful to state your intentions for next steps assuming this proposal is accepted? I know there is a section titled "Future possibilities", so let me be more specific. When I chatted with @isabela-pf about this proposal, I realized that I wasn't 100% sure if the intention was to provide a toolkit for future external extensions and future (non-existent) UI work, or if the toolkit would be a foundation for the immediate next step of converting existing UI to use the toolkit. My understanding now is that the first next step if this proposal is accepted would be using it to start converting existing JupyterLab UI.

Exactly what @echarles said 😄 Once the JEP is accepted, it will also make sense to push for it in Jupyter projects like JupyterHub, ipywidgets or nbdime.

The only other thing that really stood out for me was that I was expecting to see some discussion around performance or bundle size. Do we have some idea of how this work would impact the size or performance of the app?

For me that is out of scope - although of course we don't want to degrade performance. And a reasonable hope is to be neutral (in respect to JupyterLab core).

The positive effects are:

  • The CSS rules will likely be reduced (although this is gonna be a long work)
  • Using shadow DOM will bring the styles closer to the DOM and may ease style recomputation step.
  • Some element code within core will be reduced as the toolkit will provide some features (like keyboard navigation)
  • Web components are more efficient than React components to instantiate

The negative effects are:

  • Web components are less efficient than base html elements
  • As styles are embedded within the shadow DOM, it may increase the page size
  • Bring new dependencies in the JupyterLab bundle

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.

3 participants