-
Notifications
You must be signed in to change notification settings - Fork 21
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
Transfer search parameters from page URL to jupyterlite #108
Transfer search parameters from page URL to jupyterlite #108
Conversation
…on page to jupyterlite
Neat! Could you add an example of this in the docs? |
@@ -242,6 +257,10 @@ def run(self): | |||
prompt = self.options.pop("prompt", False) | |||
prompt_color = self.options.pop("prompt_color", None) | |||
|
|||
search_params = list( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the search_param
directive here for whitelisting the parameters that we want to transfer? Would there be an issue transferring all of them blindly?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure to understand.
It's here to build a python list from the string, but yes we could probably transfer the whole string and directly build a javascript array from it.
Do you think of an other way ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I mean is the following (removing the search_param
directive and transferring all query parameteres to the iframe)
window.jupyterliteConcatSearchParams = (iframeSrc) => {
const baseURL = window.location.origin;
const iframeUrl = new URL(iframeSrc, baseURL);
let pageParams = new URLSearchParams(window.location.search);
pageParams.keys().forEach(param => {
value = pageParams.get(param);
if (value !== null) {
iframeUrl.searchParams.append(param, value);
}
});
return iframeUrl.toString().replace(baseURL, '');
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure it makes sense to forward all query parameters. But it would simplify the code and user API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK thanks, I understand your first comment. Yes, the idea is to whitelist the parameters.
I am not sure it makes sense to forward all query parameters. But it would simplify the code and user API.
It's better to have at least the option (like a flag) to let user allow the transfer or not.
I don't know which is the best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could add an option to transfer all the parameters(=all
for example, since =
can probably not be used as parameter name), or use only a boolean value to transfer all or none.
Do you have an opinion on it @martinRenou ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, we could have the search_params be either a list of strings (whitelist) or a boolean (True=transfer all, False=transfer none).
Also, I wonder if this could be implemented as an option for other directives, instead of a global search_params
directive, like:
.. jupyterlite::
:search_params: True
Or
.. replite::
:search_params: ["theme", "code"]
This way it will be more granular
|
||
The directive `search_params` allows to transfer some search parameters from the documentation URL to the Jupyterlite URL.\ | ||
Jupyterlite will then be able to fetch these parameters from its own URL.\ | ||
For example `:search_params: ["param1", "param2"]` will transfer the parameters *param1* and *param2*. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would it make sense to provide a practical example that really applies to JupyterLite? Like "theme" for replite?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not, but I don't really know how it can be displayed to user.
The simplest solution I can find is to use something like
%%javascript
alert(window.location.search);
in a notebook, formatting a bit better the attributes.
Otherwise I didn't find a way to share value between javascript and python in a notebook, using xeus kernel.
It would be better to display the values in an output rather than using an alert
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, let's merge it as is for now!
Co-authored-by: martinRenou <[email protected]>
@brichet let's get this in! Unless you'd like to push something else? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Thanks @martinRenou for review |
This PR adds a new directive to allow transfering some search params from the documentation URL to the jupyterlite URL.
Fixes #107