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

Fixes context menu hit test to deal with svg nodes. #7242

Merged
merged 2 commits into from Sep 25, 2019

Conversation

jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Sep 19, 2019

References

Fixes #7224

Code changes

The context menu hit test now works for any node, not just html elements, though we only run the user-supplied function on html elements for backwards compatibility.

User-facing changes

The Switch Sidebar context menu item should work now.

As well, this PR includes a fix for the styling of the right sidebar icons: previously, they were being rendered upside down.

Backwards-incompatible changes

None. This should be safe to backport to 1.2 and 1.1.x.

@jasongrout
Copy link
Contributor Author

CC @telamonian

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

@jasongrout
Copy link
Contributor Author

@telamonian - can you review this?

Copy link
Member

@telamonian telamonian left a comment

Choose a reason for hiding this comment

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

LGTM!

@telamonian
Copy link
Member

We may want to revisit the decision to ignore SVGElement nodes for Jlab v2.0 (and probably give the whole context menu node hit test thing another pass in general). For now, minimizing compatibility surprises seems like a good idea.

Everything else looks good. I added a fix for the icon styling. The sidebar tab icons should now look the same on the right as they do on the left.

@jasongrout
Copy link
Contributor Author

FYI, the tests failure is an unrelated issue (apparently NASA changed its API webpage, breaking a link in our docs). I've emailed NASA about it.

@telamonian
Copy link
Member

@jasongrout I noticed about NASA. Leaving that aside, I think the PR is good to go. Mind if I merge it in?

@jasongrout
Copy link
Contributor Author

@telamonian - are your changes also safe to backport to 1.2 and 1.1.x?

@jasongrout
Copy link
Contributor Author

@telamonian - your changes look good to me. Feel free to merge if you think this is backwards compatible to 1.1.x.

@telamonian telamonian merged commit bb21bc7 into jupyterlab:master Sep 25, 2019
@telamonian
Copy link
Member

@meeseeksdev backport to 1.x

@telamonian
Copy link
Member

what do you know, the backport call actually worked

@jasongrout jasongrout modified the milestones: 1.1.x, 2.0 Sep 25, 2019
telamonian added a commit that referenced this pull request Sep 25, 2019
…2-on-1.x

Backport PR #7242 on branch 1.x (Fixes context menu hit test to deal with svg nodes.)
@lock lock bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Oct 25, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Switch Sidebar Side" context menu item doesn't work if you right click on the sidebar tab's icon
2 participants