Skip to content

Conversation

arjxn-py
Copy link
Member

@arjxn-py arjxn-py commented Apr 9, 2025

Description

Shall close #456

This PR introduces support for custom settings in JupyterGIS, addressing the current limitation of the hardcoded third-party CORS proxy used in tools.ts. The proxy URL is now configurable via the JupyterLab Settings UI, allowing users to specify their own proxy endpoint or disable it entirely.

settings.mp4

Checklist

  • PR has a descriptive title and content.
  • PR description contains references to any issues the PR resolves, e.g. Resolves #XXX.
  • PR has one of the labels: documentation, bug, enhancement, feature, maintenance
  • Checks are passing.
    Failing lint checks can be resolved with:
    • pre-commit run --all-files
    • jlpm run lint

📚 Documentation preview: https://jupytergis--619.org.readthedocs.build/en/619/
💡 JupyterLite preview: https://jupytergis--619.org.readthedocs.build/en/619/lite

Copy link
Contributor

github-actions bot commented Apr 9, 2025

Binder 👈 Launch a Binder on branch arjxn-py/jupytergis/settings

@arjxn-py
Copy link
Member Author

arjxn-py commented Apr 9, 2025

Currently having the failure -
image

cc: @martinRenou

@arjxn-py arjxn-py added the enhancement New feature or request label Apr 9, 2025
@arjxn-py arjxn-py marked this pull request as draft April 9, 2025 10:04
Copy link
Contributor

github-actions bot commented Apr 9, 2025

Integration tests report: appsharing.space

"access": "public"
},
"jupyterlab": {
"schemaDir": "schema",
Copy link
Member

Choose a reason for hiding this comment

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

Line 17 of this file, you want to list the schema folder as part of the package file

@arjxn-py arjxn-py changed the title [WIP]: Settings to configure proxy url Settings to configure proxy url Apr 28, 2025
@arjxn-py arjxn-py marked this pull request as ready for review April 28, 2025 13:50
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks!!

Here you are using a global settingsState. This will not work well for listening to settings changes.

Instead you should probably pass around the settingRegistry to the JupyterGISModelFactory, and add a new constructor option to the JupyterGISModel so that it can own the settingRegistry and listen to setting changes there.

@arjxn-py arjxn-py requested a review from martinRenou April 29, 2025 08:57
@arjxn-py arjxn-py changed the title Settings to configure proxy url Introduce Custom Settings for JupyterGIS Apr 30, 2025
@arjxn-py
Copy link
Member Author

image

Failure diff fow lite deployment on this PR, strange because when i check it works fine on RTD

@mfisher87
Copy link
Member

@arjxn-py could you write a description for this PR?

@arjxn-py
Copy link
Member Author

arjxn-py commented May 5, 2025

@arjxn-py could you write a description for this PR?

Done, thanks to recall that :)

@mfisher87
Copy link
Member

Thanks for adding it :)

@arjxn-py arjxn-py closed this May 6, 2025
@arjxn-py arjxn-py reopened this May 6, 2025
Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks!

contentsManager: Contents.IManager | undefined;
filePath: string;

getSettings(): any;
Copy link
Member

Choose a reason for hiding this comment

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

Can we get better typing than this?

Copy link
Member

Choose a reason for hiding this comment

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

Especially, the any is shadowing the fact that it can be null.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, i'll look into this

@arjxn-py arjxn-py requested a review from martinRenou May 6, 2025 16:32
@arjxn-py
Copy link
Member Author

arjxn-py commented May 6, 2025

The failure is legit, i'll look into it

Copy link
Member

@martinRenou martinRenou left a comment

Choose a reason for hiding this comment

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

Thanks!!!!

@martinRenou martinRenou merged commit 5b440ca into geojupyter:main May 7, 2025
14 checks passed
@arjxn-py arjxn-py deleted the settings branch May 7, 2025 14:58
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.

Make CORS proxy configurable
3 participants