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

Commands refactor #86

Merged
merged 7 commits into from
Aug 2, 2024
Merged

Commands refactor #86

merged 7 commits into from
Aug 2, 2024

Conversation

gjmooney
Copy link
Collaborator

@gjmooney gjmooney commented Aug 1, 2024

This replaces all the separate commands we had for creating sources and layers with one createEntry that takes all the necessary arguments to pass to the CreationFormDialog, and I attempted to standardize how we handle icons and labels. I also wanted to have a consistent naming pattern so I changed the names of the commands that create a source and layer at the same time to new{type}Entry to differentiate those from the source/layer only commands. This also re-orders some functions to try and keep the order consistent between our constants, commands, context menus, and the toolbar.

Copy link
Contributor

github-actions bot commented Aug 1, 2024

Binder 👈 Launch a Binder on branch gjmooney/jupytergis/commands_refactor

Copy link
Collaborator

@brichet brichet left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good.

I just have a comment below.

EDIT:
Actually there are a bunch of functions that may be removed from the Private namespace of commands.ts, IIUC

  • createRasterDemSource
  • createVideoSource
  • createImageSource
  • createVectorLayer
  • createHillshadeLayer
  • createVideoLayer
  • createImageLayer

Comment on lines 203 to 238
// Layer and group actions
palette.addItem({
command: CommandIDs.renameLayer,
category: 'JupyterGIS'
});

palette.addItem({
command: CommandIDs.removeLayer,
category: 'JupyterGIS'
});

palette.addItem({
command: CommandIDs.renameGroup,
category: 'JupyterGIS'
});

palette.addItem({
command: CommandIDs.removeGroup,
category: 'JupyterGIS'
});

palette.addItem({
command: CommandIDs.moveLayerToNewGroup,
category: 'JupyterGIS'
});

// Source actions
palette.addItem({
command: CommandIDs.renameSource,
category: 'JupyterGIS'
});

palette.addItem({
command: CommandIDs.removeSource,
category: 'JupyterGIS'
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Wondering if we want to add these ones to the palette.
These commands need to have an item selected in side panel. But when the item is selected, it seems easier to type Del or F2 instead of opening the command palette and search for the command...

@gjmooney
Copy link
Collaborator Author

gjmooney commented Aug 1, 2024

EDIT: Actually there are a bunch of functions that may be removed from the Private namespace of commands.ts, IIUC

* `createRasterDemSource`

* `createVideoSource`

* `createImageSource`

* `createVectorLayer`

* `createHillshadeLayer`

* `createVideoLayer`

* `createImageLayer`

I thought I got rid of those, must've snuck back in when I rebased 😂

That's a good point about the palette commands, I'll take out the ones with keyboard shortcuts.

@brichet
Copy link
Collaborator

brichet commented Aug 2, 2024

I don't know if this PR is ready yet, but currently it seems that some layers have a wrong icon

image

@gjmooney
Copy link
Collaborator Author

gjmooney commented Aug 2, 2024

I don't know if this PR is ready yet, but currently it seems that some layers have a wrong icon

image

Images and videos are raster layers. I was thinking we could add an icon or sourceType or something like that to the IJGISLayer and load the icon from that here.

@brichet
Copy link
Collaborator

brichet commented Aug 2, 2024

Images and videos are raster layers. I was thinking we could add an icon or sourceType or something like that to the IJGISLayer and load the icon from that here.

OK, I didn't realize that.
I think that we should keep it that way for now, to avoid some unnecessary complexity in code.

Copy link
Collaborator

@brichet brichet left a comment

Choose a reason for hiding this comment

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

Thanks @gjmooney, this looks good

@brichet brichet merged commit 7cc268b into geojupyter:main Aug 2, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants