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.

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

Add subitems to layers (either layers or controls or a mixture of both) #1011

Merged
merged 40 commits into from Sep 23, 2022
Merged

Add subitems to layers (either layers or controls or a mixture of both) #1011

merged 40 commits into from Sep 23, 2022

Conversation

HaudinFlorence
Copy link
Contributor

@HaudinFlorence HaudinFlorence commented Jul 1, 2022 •
edited

Add subitems to layers (either layers or controls or a mixture of both).

@davidbrochart davidbrochart changed the title Add subitems to layers (either layers or controls or a mixture or both). Add subitems to layers (either layers or controls or a mixture of both) Jul 4, 2022
davidbrochart
davidbrochart reviewed Aug 9, 2022
View reviewed changes
Copy link
Member

@davidbrochart davidbrochart left a comment

Choose a reason for hiding this comment

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

Could you rebase on master?

examples/Choropleth.ipynb Outdated Show resolved Hide resolved
examples/Subitems.ipynb Outdated Show resolved Hide resolved
ipyleaflet/leaflet.py Outdated Show resolved Hide resolved
ipyleaflet/leaflet.py Outdated Show resolved Hide resolved
@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Aug 10, 2022 •
edited

Could you rebase on master?

Thanks for the review. Rebase has been made.

@HaudinFlorence HaudinFlorence force-pushed the add_subitems_to_layers branch 2 times, most recently from 38dfc81 to 1aa9b8f Compare September 5, 2022 12:02
martinRenou
martinRenou reviewed Sep 5, 2022
View reviewed changes
js/src/Map.js Outdated Show resolved Hide resolved
js/src/Map.js Outdated Show resolved Hide resolved
js/src/Map.js Outdated Show resolved Hide resolved
ipyleaflet/leaflet.py Outdated Show resolved Hide resolved
martinRenou
martinRenou requested changes Sep 6, 2022
View reviewed changes
js/src/Map.js Outdated Show resolved Hide resolved
js/src/Map.js Outdated Show resolved Hide resolved
js/src/Map.js Outdated Show resolved Hide resolved
js/src/Map.js Outdated Show resolved Hide resolved
js/src/Map.js Outdated Show resolved Hide resolved
martinRenou
martinRenou requested changes Sep 6, 2022
View reviewed changes
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.

It would be great to add a galata test for this :)

js/src/Map.js Outdated Show resolved Hide resolved
js/src/Map.js Outdated Show resolved Hide resolved
ipyleaflet/leaflet.py Outdated Show resolved Hide resolved
@HaudinFlorence
Copy link
Contributor Author

It would be great to add a galata test for this :)

A notebook has been added in ui-tests/notebooks in e0b19ac, to test adding subitems in layers and check that they correctly display.

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Sep 7, 2022 •
edited

please update galata references

@HaudinFlorence
Copy link
Contributor Author

Please update galata reference

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Sep 12, 2022 •
edited

please update galata references

@HaudinFlorence
Copy link
Contributor Author

please update galata references

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Sep 15, 2022 •
edited

The CI tests are now green. Is there other things I may add or check ?

@martinRenou
Copy link
Member

Thanks! I will give the PR a try. I suspect there is a bug when you create the map in a first Notebook cell, and add the layers with sub-layers in a separate cell.

martinRenou
martinRenou requested changes Sep 15, 2022
View reviewed changes
setup.py Outdated Show resolved Hide resolved
@martinRenou
Copy link
Member

I suspect there is a bug when you create the map in a first Notebook cell, and add the layers with sub-layers in a separate cell.

Indeed there is an issue when the Map is shown before adding the layers with sub-items, as seen in this screenshot (the first map view does not have the subitems visible, the second map view has them):

Screenshot from 2022-09-15 11-48-07

martinRenou
martinRenou requested changes Sep 20, 2022
View reviewed changes
ipyleaflet/leaflet.py Outdated Show resolved Hide resolved
js/src/layers/Layer.js Outdated Show resolved Hide resolved
js/src/layers/Layer.js Show resolved Hide resolved
js/src/layers/Layer.js Show resolved Hide resolved
@HaudinFlorence
Copy link
Contributor Author

please update galata references

1 similar comment
@HaudinFlorence
Copy link
Contributor Author

please update galata references

HaudinFlorence and others added 13 commits September 22, 2022 19:18
@HaudinFlorence
Re try CI tests.
30558c3
@github-actions @HaudinFlorence
Update Playwright Snapshots
b62bf9a
@HaudinFlorence
Fix the failing ui-test by setting spin to false for the icon1 Awesom…
5c1ede7 
…eIcon subitem, in ui-tests/notebooks/Subitems.ipynb.
@HaudinFlorence
Remove the marker with an icon and replace it by a simple one.
c992a40
@HaudinFlorence
Restore the marker with the icon.
e53dc31
@github-actions @HaudinFlorence
Update Playwright Snapshots
70aa0b7
@HaudinFlorence
Retry CI tests after the update of the galata references.
7bedabf
@HaudinFlorence
Update Layer.js and Map.js to deal with the model and view of subitem…
57c8fa1 
…s at the scale of the layer.
@HaudinFlorence
Update Layer.js and the example notebook Subitems.ipynb.
88f78fc
@HaudinFlorence @martinRenou
Update setup.py
b4f91bb 
Co-authored-by: martinRenou <martin.renou@gmail.com>
@HaudinFlorence
Add pandas dependency in tests and fix linting in Layers.js.
e4b2804
@HaudinFlorence
Remove pandas version in tests dependencies.
095ff12
@HaudinFlorence
Change layer2 basemap in ui-tests/notebooks/Subitems, to try to fix t…
c52fc55 
…he failing rendering widgets test.
@HaudinFlorence
Copy link
Contributor Author

please update galata references

1 similar comment
@martinRenou
Copy link
Member

please update galata references

@martinRenou
Copy link
Member

please update galata references

@martinRenou
Copy link
Member

Please update galata references, this time it should probably work :D

@martinRenou
Copy link
Member

update galata references

@martinRenou
Copy link
Member

Triggering CI

@martinRenou martinRenou reopened this Sep 23, 2022
martinRenou
martinRenou approved these changes Sep 23, 2022
View reviewed changes
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.

Thank you!!!

@martinRenou martinRenou merged commit 50dc472 into jupyter-widgets:master Sep 23, 2022
@HaudinFlorence
Copy link
Contributor Author

Thank you!!!

Thanks a lot for the review and help on this PR !

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

@davidbrochart davidbrochart davidbrochart left review comments

@martinRenou martinRenou martinRenou approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@HaudinFlorence @martinRenou @davidbrochart