Skip to content

Enable empty path for geojson layer #810

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Aug 4, 2025
Merged

Enable empty path for geojson layer #810

merged 8 commits into from
Aug 4, 2025

Conversation

HaudinFlorence
Copy link
Contributor

@HaudinFlorence HaudinFlorence commented Jul 11, 2025 •
edited
Loading

Description

Create an empty vector layer with empty embedded data when no path is provided

Screencast.From.2025-07-11.16-00-04.mp4

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

@github-actions GitHub Actions
Copy link
Contributor

Binder 👈 Launch a Binder on branch HaudinFlorence/jupytergis/enable_empty_path_for_geojson_layer

@HaudinFlorence HaudinFlorence force-pushed the enable_empty_path_for_geojson_layer branch from 2f4e6b8 to 84f9d5d Compare July 11, 2025 14:06
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jul 11, 2025 •
edited
Loading

Integration tests report: appsharing.space

@HaudinFlorence HaudinFlorence self-assigned this Jul 11, 2025
@HaudinFlorence HaudinFlorence added the enhancement New feature or request label Jul 11, 2025
@mfisher87
Copy link
Member

How can we indicate to the user that this is what is happening?

What about a checkbox "Create an empty GeoJSON layer. I will add my own data with the draw tool."?

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Jul 11, 2025 •
edited
Loading

How can we indicate to the user that this is what is happening?

What about a checkbox "Create an empty GeoJSON layer. I will add my own data with the draw tool."?

You are right that it would be more explicit to better inform the user. I will try to propose something.

@HaudinFlorence
Copy link
Contributor Author

We previously decided to propose to add this message in brackets If no path/url is provided, an empty GeoJSON is created, but that's probably not enough.

@mfisher87
Copy link
Member

mfisher87 commented Jul 11, 2025 •
edited
Loading

Dynamic "Field A || Field B" interactions are one of those cases that I think is really hard with our generate-forms-from-schemas technique. I'm not sure how to resolve this yet. Maybe this is something we can brainstorm together at a hackathon!

@HaudinFlorence HaudinFlorence force-pushed the enable_empty_path_for_geojson_layer branch 2 times, most recently from 5b5818a to eca455f Compare July 15, 2025 08:47
@martinRenou martinRenou force-pushed the enable_empty_path_for_geojson_layer branch from eca455f to 45a4f00 Compare July 15, 2025 12:49
@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@martinRenou
Copy link
Member

@HaudinFlorence please undo the bot's commit, it's not using the right command, resulting in those files being added with the wrong names.

We should update the bots to use the latest of https://github.com/geojupyter/jupytergis/blob/main/.github/workflows/update_galata_references.yaml#L104 (replace v1 -> main like in https://github.com/jupytercad/JupyterCAD/blob/main/.github/workflows/update_lite_galata_references.yaml)

@HaudinFlorence HaudinFlorence force-pushed the enable_empty_path_for_geojson_layer branch 2 times, most recently from 45a4f00 to 92356d9 Compare July 15, 2025 14:49
@HaudinFlorence
Copy link
Contributor Author

@HaudinFlorence please undo the bot's commit, it's not using the right command, resulting in those files being added with the wrong names.

We should update the bots to use the latest of https://github.com/geojupyter/jupytergis/blob/main/.github/workflows/update_galata_references.yaml#L104 (replace v1 -> main like in https://github.com/jupytercad/JupyterCAD/blob/main/.github/workflows/update_lite_galata_references.yaml)

Thanks for the feedback. This has been fixed in #816

@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@HaudinFlorence HaudinFlorence force-pushed the enable_empty_path_for_geojson_layer branch 3 times, most recently from 1d7c10e to f48548e Compare July 16, 2025 08:28
@HaudinFlorence HaudinFlorence force-pushed the enable_empty_path_for_geojson_layer branch from f48548e to 3534399 Compare July 17, 2025 14:15
martinRenou
martinRenou reviewed Jul 17, 2025
View reviewed changes
packages/base/src/tools.ts Outdated
Comment on lines 502 to 504
if (!filepath) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this?

It seems to me we should raise an error if that's the case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can remove this

@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@HaudinFlorence HaudinFlorence force-pushed the enable_empty_path_for_geojson_layer branch from b3f143e to 6a46d40 Compare July 17, 2025 16:33
@HaudinFlorence HaudinFlorence force-pushed the enable_empty_path_for_geojson_layer branch from 6a46d40 to 3f72f16 Compare August 1, 2025 12:29
@HaudinFlorence HaudinFlorence force-pushed the enable_empty_path_for_geojson_layer branch from 812dd0e to 6b4bf86 Compare August 4, 2025 09:59
arjxn-py
arjxn-py reviewed Aug 4, 2025
View reviewed changes
arjxn-py
arjxn-py reviewed Aug 4, 2025
View reviewed changes
gjmooney
gjmooney approved these changes Aug 4, 2025
View reviewed changes
@HaudinFlorence @gjmooney @arjxn-py
Apply suggestions from code review
7b16e7e 
Co-authored-by: Greg Mooney <[email protected]>
Co-authored-by: Arjun Verma <[email protected]>
arjxn-py
arjxn-py approved these changes Aug 4, 2025
View reviewed changes
Copy link
Member

@arjxn-py arjxn-py left a comment

Choose a reason for hiding this comment

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

Thanks @HaudinFlorence

@arjxn-py arjxn-py merged commit b294c7d into geojupyter:main Aug 4, 2025
14 checks passed
@HaudinFlorence
Copy link
Contributor Author

@gjmooney @arjxn-py Thanks for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@martinRenou martinRenou martinRenou left review comments

@gjmooney gjmooney gjmooney approved these changes

@arjxn-py arjxn-py arjxn-py approved these changes

Assignees

@HaudinFlorence HaudinFlorence

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@HaudinFlorence @mfisher87 @martinRenou @gjmooney @arjxn-py