fix: remove iframe in support of iframeless rendering #139
Conversation
There was a problem hiding this comment.
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
maidr/core/maidr.py:193
- The new parameter 'maidr_id' in _inject_plot lacks an explicit type annotation and defensive checks. It is used in string interpolation in the JavaScript code, so adding a type hint (e.g., str) and a check to ensure it's not None could prevent potential runtime issues.
def _inject_plot(plot: HTML, maidr: str, maidr_id) -> Tag:
maidr/core/maidr.py
Outdated
| @@ -91,6 +94,8 @@ def show(self, renderer: Literal["auto", "ipython", "browser"] = "auto") -> obje | |||
| Environment.is_interactive_shell() and not Environment.is_notebook() | |||
| ): | |||
| return self._open_plot_in_browser() | |||
| if Environment.is_notebook(): | |||
| Javascript("Jupyter.keyboard_manager.disable();") | |||
There was a problem hiding this comment.
The call to Javascript('Jupyter.keyboard_manager.disable();') is made without wrapping it in display(), which may prevent the JavaScript from executing properly in an IPython notebook environment. Consider using display(Javascript(...)) to ensure the script runs as intended.
| Javascript("Jupyter.keyboard_manager.disable();") | |
| display(Javascript("Jupyter.keyboard_manager.disable();")) |
Copilot uses AI. Check for mistakes.
dakshpokar
changed the title
There was a problem hiding this comment.
PR Overview
This pull request removes iframe rendering by introducing a unique identifier (maidr_id) and updating the injection logic for the plot and related scripts. Key changes include:
- Adding and propagating a new maidr_id attribute to uniquely identify SVG elements.
- Updating the _inject_plot method to support the new maidr_id parameter and script initialization.
- Introducing a new static method is_vscode_notebook in the environment module.
Reviewed Changes
| File | Description |
|---|---|
| maidr/core/maidr.py | Added maidr_id, updated _inject_plot signature and SVG attribute replacement logic. |
| maidr/util/environment.py | Added is_vscode_notebook static method to detect VSCode notebooks. |
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| def is_vscode_notebook() -> bool: | ||
| """Return True if the environment is a VSCode notebook.""" | ||
| try: | ||
| if "VSCODE_PID" in os.environ or "VSCODE_JUPYTER" in os.environ: |
There was a problem hiding this comment.
The os module is used in is_vscode_notebook but is not imported. Please add 'import os' at the top of the file.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
PR Overview
This PR implements an iframe-free solution by introducing a unique identifier (maidr_id) for each Maidr instance and updating the rendering logic accordingly, while still retaining iframes for specific notebook environments.
- Added a maidr_id attribute and updated its propagation throughout the rendering process.
- Updated the _inject_plot method to include maidr_id and adjusted the SVG and MAIDR data injection accordingly.
- Introduced a new method is_vscode_notebook in the environment utility for future notebook detection.
Reviewed Changes
| File | Description |
|---|---|
| maidr/core/maidr.py | Added handling for maidr_id, updated SVG injection, and modified iframe embedding logic. |
| maidr/util/environment.py | Added a new static method to detect VSCode notebooks. |
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| height += 100; | ||
| }}else{{ | ||
| height += 50 | ||
| is_quarto = os.getenv("IS_QUARTO") == "True" |
There was a problem hiding this comment.
The os module is used here but it is not imported in this file. Consider adding an import statement for os to prevent runtime errors.
Copilot uses AI. Check for mistakes.
Description
This pull request introduces an iframe-free solution. However, it cannot be fully implemented yet because keybindings in Jupyter Notebook, Google Colab, and VSCode's IPython Notebook still need to be fixed. Once we resolve the keybindings issue, we can remove the conditional check that currently renders maidr inside an iframe, and the solution will function seamlessly. For now, plots on these platforms will continue to be rendered within an iframe.
NOTE: It's very crucial to first merge the maidr.js PR before merging this PR
xability/maidr#637
addesses #108
Type of Change
Changes
Checklist