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

Think of dialog to confirm whether to load JupyterLight #144

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Carreau
Copy link
Collaborator

@Carreau Carreau commented Mar 5, 2024

This is not perfect, in particular the confirm(...) is a blocking call, which is not supposed to be used in a OnClik handler as an onclick handler need to respond fast.

I think this need to be refactored into utilities functions, and setTimeout(0, callback), and/or Promises.

That way the onClick finishes immediately and does not violate the timeout limit.

We can't use SetTimeout either as it also suffer the same problem. For now I think we can live with the warning in the console on slow or metered connections.

As noted in the comment, the navigator api is experimental, so it would be good to test properly. In Web Inspector (Firefox and Chome at least), it is possible to artificially throttle the connection to test the logic.

@Carreau Carreau requested a review from steppi March 5, 2024 10:32
@Carreau Carreau added the enhancement New feature or request label Mar 6, 2024
@Carreau Carreau marked this pull request as draft March 6, 2024 09:04
@Carreau Carreau force-pushed the launch-dialog branch 3 times, most recently from 4a3b25c to 35141e5 Compare March 6, 2024 10:28
As noted in the comment, the navigator api is experimental, so it woudl
be good to test properly. In Web Inspector  (Firefox and Chome at
least), it is possible to artificially throttle the connection to test
the logic.
@Carreau Carreau marked this pull request as ready for review March 6, 2024 10:29
@Carreau Carreau changed the title Draft: Think of dialog to confirm whether to load JupyterLight Think of dialog to confirm whether to load JupyterLight Mar 6, 2024
@steppi
Copy link
Collaborator

steppi commented Mar 7, 2024

I think the downloads only happen if someone tries to execute a cell with imports in it. I wonder if it would be sufficient to add a note about large downloads in the initial warning cell? My impression is Apple has no intention of ever implementing navigator, so only usingnavigator seems like it might be an unworkable solution in practice.

@martinRenou
Copy link
Member

I think the downloads only happen if someone tries to execute a cell with imports in it.

Just adding a note that this is true for the pyodide kernel, not true for the xeus-python kernel which would download the Python environment at kernel startup. So the first cell approach does not fit the xeus-python case, because the kernel would have started as soon as you open the notebook.

@steppi
Copy link
Collaborator

steppi commented Mar 7, 2024

I think the downloads only happen if someone tries to execute a cell with imports in it.

Just adding a note that this is true for the pyodide kernel, not true for the xeus-python kernel which would download the Python environment at kernel startup. So the first cell approach does not fit the xeus-python case, because the kernel would have started as soon as you open the notebook.

Thanks, that’s good to know. I want to add the warning about large downloads when I deploy for SciPy’s docs, but it seems we still need a principled solution.

@Carreau
Copy link
Collaborator Author

Carreau commented Mar 18, 2024

Sorry for the delay in responding. I think we can design the mechanism we wish to alert user and iterate over the API and predicate we use to who it.

Note that the "download" buttons already should be hidden on small screens, so this will likely not be an issue on phones.

I also think that even on mobile it might not be a problem to download a non-negligible amount of data. I don't see many people trying scientific computing from a low power machine with a slow connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants