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

Not clear how to support Panel extensions in 1.0 #4957

Open
MarcSkovMadsen opened this issue May 28, 2023 · 11 comments
Open

Not clear how to support Panel extensions in 1.0 #4957

MarcSkovMadsen opened this issue May 28, 2023 · 11 comments
Assignees
Labels
type: bug Something isn't correct or isn't working

Comments

@MarcSkovMadsen
Copy link
Collaborator

MarcSkovMadsen commented May 28, 2023

I'm trying to upgrade panel-chemistry to Panel 1.0 in #41 and I cannot get it working. I see the following 2 issues when I serve the test_jsme_editor app.

image

Somehow I believe the JSMEEditor Bokeh model is not being registered any more and the https://unpkg.com/[email protected]/jsme.nocache.js file not being loaded. But its clear to me exactly what the cause and solution is.

I need help.

Reproduce

Create and activate a virtual environment

conda create --name panel-chemistry python=3.10 nodejs
conda activate panel-chemistry

Clone the repo and checkout the branch

git clone https://github.com/awesome-panel/panel-chemistry.git
cd panel-chemistry
git checkout feature/support-panel-1.0

Install the dependencies

pip install pip -U
pip install -e .[dev,examples]

Build the bokeh models

panel build src/panel_chemistry

Serve the app

panel serve tests/tests/test_jsme_editor.py --autoreload

Verify the issue

If more info is needed, see the DEVELOPER_GUIDE

@pedroarbs
Copy link

pedroarbs commented Jun 7, 2023

It would be great if awesome-panel-extensions also supported panel version 1.0. I use panel/holoviews for many data science projects, and awesome-panel provides a very elegant portfolio for the projects.

@krairy
Copy link

krairy commented Jul 5, 2023

I am a new user of Panel. It would be truly awesome if awesome-panel worked on Panel <= 1.0!

@mattpap
Copy link
Collaborator

mattpap commented Aug 29, 2023

To fix the "unknown property" error:

https://github.com/awesome-panel/panel-chemistry/blob/da0f00950ca6430e04c2a084d625153e6079f3ea/src/panel_chemistry/bokeh_extensions/ngl_viewer.ts#L198

static init_NGLViewer(): void {

This needs to be rewritten as static { (a static block). We previously emulated static blocks with static functions and a compiler plugin that injected code that called those functions. Given that TypeScript now supports static blocks, this was made obsolete and support was removed in 3.0.

There are more issues, but I need to look in this in more detail before I can give concrete advice.

@mattpap mattpap self-assigned this Aug 29, 2023
@MikeHeiber
Copy link

My team and I would also like to use the JSME molecule editor widget in modern panel apps. @mattpap are you working on this? How challenging do you think the fix is to implement? Any estimate on when a fix would be completed? Perhaps we can help in some way?

@philippjfr
Copy link
Member

Should be a relatively simple update, but we have limited bandwidth to look at this. By comparing the model definitions in panel/models and applying the same to the panel-chemistry models it should be relatively simple to update.

@hoxbro
Copy link
Member

hoxbro commented Nov 9, 2023

I have made an initial update to JSME here: awesome-panel/panel-chemistry#42

@MarcSkovMadsen
Copy link
Collaborator Author

Thx. I'll look at this on weekends

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Nov 25, 2023

My next pain point is the NGL viewer. I can render it by first rendering outside of shadowroot and then appending to the shadow_el. But after moving it inside shadow_el mouse events like drag and zoom no longer works.

I've reported it with NGL in nglviewer/ngl#1003 with a minimum, reproducible example. But maybe issues like these are familiar to Bokeh or Panel developers like @mattpap or @philippjfr? Please let me know. Thanks

@mattpap
Copy link
Collaborator

mattpap commented Nov 25, 2023

But after moving it inside shadow_el mouse events like drag and zoom no longer works.

Problems with event handling are typically related to using Event.target, which doesn't work as expected anymore when shadow DOM is involved. See e.g. here:

https://github.com/nglviewer/ngl/blob/d9b2914527f66ff4033778542843c321d8a88264/src/stage/mouse-observer.ts#L302-L305

    if (event.target !== this.domElement) {
      return
    }

Though every handler in that module is affected. Typically this is solved by rewriting such code as:

    if (!event.composedPath().includes(this.domElement)) {
      return
    }

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Nov 25, 2023

Thx. @mattpap. Is it correctly understood this needs to be done on the nglviewer side? I cannot do it in my code?

@MarcSkovMadsen
Copy link
Collaborator Author

MarcSkovMadsen commented Nov 27, 2023

I can see that in nglviewer/ngl#1003 (comment) they propose for us to use slots. I would try to see if that could work. But my brain says no because we are inside deeply nested shadow roots.

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Something isn't correct or isn't working
Projects
None yet
Development

No branches or pull requests

7 participants