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.

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

Minor visual bug fixes #1754

Merged
merged 11 commits into from Oct 4, 2023
Merged

Minor visual bug fixes #1754

merged 11 commits into from Oct 4, 2023

Conversation

naschmitz
Copy link
Collaborator

The bug fixes are as follows:

  • Fix opening URLs in a Colab environment. I'm displaying JS code that calls window.open.
  • Disable toggling the basemap layer. This is really unexpected behavior, especially because the basemap layer doesn't show up in the list of layers. I think the right solution is to show the basemap in the list of layers.
  • Fix extra vspace below toolbar buttons.

@github-actions
Copy link

github-actions bot commented Oct 3, 2023

@github-actions github-actions bot temporarily deployed to pull request October 3, 2023 13:41 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 3, 2023 20:44 Inactive
@github-actions github-actions bot temporarily deployed to pull request October 3, 2023 21:17 Inactive
Copy link
Collaborator

@sufyanAbbasi sufyanAbbasi left a comment

Choose a reason for hiding this comment

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

Looks great! I had one comment regarding window.open patterns.

geemap/common.py Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to pull request October 4, 2023 02:10 Inactive
@giswqs
Copy link
Member

giswqs commented Oct 4, 2023

Should we make the basemap layer show up on the layer manager? Right now, the basemap is always shown and can't be toggled off. This is not ideal because sometime users just want to show some EE layers for a small non-rectangular region without basemap in the background.

Peek.2023-10-03.22-35.mp4

@naschmitz
Copy link
Collaborator Author

Should we make the basemap layer show up on the layer manager? Right now, the basemap is always shown and can't be toggled off. This is not ideal because sometime users just want to show some EE layers for a small non-rectangular region without basemap in the background.

Peek.2023-10-03.22-35.mp4

Is it alright if we address this in a future PR? I think the best solution is to include the basemap in the layer manager, but it doesn't seem like a launch-blocking issue.

@giswqs
Copy link
Member

giswqs commented Oct 4, 2023

I have included the basemap in the layer manger. Only one line of code needs to be changed. The only minor issue is that the basemap name OpenStreetMap.Mapnik is a bit long for the layer manager, and it is shown as OpenStreetMap....

If you are OK with that, I can commit the changes. Or do we want the rename the default basemap from OpenStreetMap.Mapnik to OpenStreetMap so that the three dots do not show up.

image

Peek.2023-10-03.22-52.mp4

@naschmitz
Copy link
Collaborator Author

I think it's fine if the three dots show up. Thanks Qiusheng!

@naschmitz
Copy link
Collaborator Author

You'll need to update the unit test too.

@giswqs
Copy link
Member

giswqs commented Oct 4, 2023

I am looking into the unit tests now. Will fix it shortly.

@github-actions github-actions bot temporarily deployed to pull request October 4, 2023 03:31 Inactive
@giswqs
Copy link
Member

giswqs commented Oct 4, 2023

The basemap name issue is fixed. The three dots no longer show up. Will merge this shortly and make a minor release.

image

@giswqs giswqs merged commit 9d622af into master Oct 4, 2023
13 checks passed
@giswqs giswqs deleted the nschmitz-bug-fixes branch October 4, 2023 03:41
@github-actions github-actions bot temporarily deployed to pull request October 4, 2023 03:43 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants