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

@jasongrout jasongrout commented Sep 19, 2019

CC @telamonian

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Sep 19, 2019

Thanks for making a pull request to JupyterLab!

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Sep 23, 2019

@telamonian - can you review this?

Copy link
Member

@telamonian telamonian left a comment

LGTM!

@telamonian
Copy link
Member

@telamonian telamonian commented Sep 24, 2019

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

@jasongrout jasongrout commented Sep 25, 2019

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

@telamonian telamonian commented Sep 25, 2019

@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

@jasongrout jasongrout commented Sep 25, 2019

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

@jasongrout
Copy link
Contributor Author

@jasongrout jasongrout commented Sep 25, 2019

@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
7 of 9 checks passed
@telamonian
Copy link
Member

@telamonian telamonian commented Sep 25, 2019

@meeseeksdev backport to 1.x

@telamonian
Copy link
Member

@telamonian telamonian commented Sep 25, 2019

what do you know, the backport call actually worked

@jasongrout jasongrout removed this from the 1.1.x milestone Sep 25, 2019
@jasongrout jasongrout added this to the 2.0 milestone Sep 25, 2019
telamonian added a commit that referenced this issue 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 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants