Skip to content

Conversation

@gjmooney
Copy link
Collaborator

@gjmooney gjmooney commented Nov 6, 2025

Description

Resolve #910

Remove DOM related stuff from layer selection, allow setting selection programmatically

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--981.org.readthedocs.build/en/981/
💡 JupyterLite preview: https://jupytergis--981.org.readthedocs.build/en/981/lite

@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

Binder 👈 Launch a Binder on branch gjmooney/jupytergis/selection-refactor

@gjmooney gjmooney added the enhancement New feature or request label Nov 6, 2025
@github-actions
Copy link
Contributor

github-actions bot commented Nov 6, 2025

Integration tests report: appsharing.space

@gjmooney gjmooney marked this pull request as ready for review November 12, 2025 13:14
Comment on lines 540 to 544
handleItemSelection(
type: SelectionType,
item: string,
event?: { ctrlKey: boolean; button: number },
): { [key: string]: ISelection } | null {
Copy link
Member

@martinRenou martinRenou Nov 12, 2025

Choose a reason for hiding this comment

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

I would keep event-related things out of the model, and handle those in the view only:

Suggested change
handleItemSelection(
type: SelectionType,
item: string,
event?: { ctrlKey: boolean; button: number },
): { [key: string]: ISelection } | null {
set selection(
newSelection: { [key: string]: ISelection } | null
) {
this.localState?.selected?.value = newSelection;
}
get selection(): { [key: string]: ISelection } | null {
return this._selection;
}

From the view that handles the event:

const currentSelection = model.selection;
if (ctrlKey) {
  model.selection = {...currentSelection, newItem}
}

@mfisher87
Copy link
Member

Hey Greg, would you mind updating the PR description to include a bit more information about the motivation for this change, and the title to be a bit more specific? 🙇

@gjmooney gjmooney marked this pull request as draft November 13, 2025 09:56
@gjmooney gjmooney changed the title Refactor item selection Remove DOM nonsense from selection logic Nov 13, 2025
@gjmooney gjmooney marked this pull request as ready for review November 13, 2025 11:53
@gjmooney gjmooney requested a review from martinRenou November 13, 2025 11:53
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 d2400b6 into geojupyter:main Nov 13, 2025
16 checks passed
@gjmooney gjmooney deleted the selection-refactor branch November 13, 2025 12:09
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

Status: Done

Development

Successfully merging this pull request may close these issues.

Refactor layer selection logic: make the model responsible for selection

3 participants