-
Notifications
You must be signed in to change notification settings - Fork 345
compatibility with jupyterlab >= 2.0 #599
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
Conversation
@fcollonval I rebased the PR #550 on master and fixed a couple of issues. All tests pass. Could you review? |
@btel I have added a couple of minor changes, and at a first glance it's looking good. Let me spend a little more time with the applied changes. |
thanks @lresende. It looks good. Why do we need to force enable extensions here? Shouldn't it be applied in another PR to keep the single scope of this one? |
@btel Agree with the assessment for the last commit. How do you want to handle that? Revert or Remove + push force? After your preference, I can update the branch... |
@lresende it's done! I just removed and force-pushed the branch. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@btel Thanks for this. I added some suggestions to improve dependency version pins.
@lresende I have the rights to publish. But unfortunately, my rights on this GitHub project are too low for me to carry modifications. Could you increase them so I could publish a new version?
Note: I'ld like a couple of small PR including fixes to be merged first in order to publish a patch version for JLab 1 before merging this one and publishing 0.20.
@fcollonval thanks for the comments. I will integrate them tonight. There is no problem with waiting for the fixes to be published before merging, but it's better not to wait to long so the master does not diverge too much from this PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I checked this out and found a few icon issues. I figured out fixes for them, two I've included as suggested changed. The rest are with with css variables.
Rather than explain, I've included a git diff of my suggested changes, most revolve around icon var name changes and some css updates to match the "new" icons.
Git diff (collapsed for readability)
diff --git a/src/components/SinglePastCommitInfo.tsx b/src/components/SinglePastCommitInfo.tsx
index 2900fff..6d7194c 100644
--- a/src/components/SinglePastCommitInfo.tsx
+++ b/src/components/SinglePastCommitInfo.tsx
@@ -171,7 +171,7 @@ export class SinglePastCommitInfo extends React.Component<
<span>
<insertionsMadeIcon.react
className={classes(iconClass, insertionsIconClass)}
- tag="span"
+ tag="div"
title="# Insertions"
/>
{this.state.insertions}
@@ -179,7 +179,7 @@ export class SinglePastCommitInfo extends React.Component<
<span>
<deletionsMadeIcon.react
className={classes(iconClass, deletionsIconClass)}
- tag="span"
+ tag="div"
title="# Deletions"
/>
{this.state.deletions}
diff --git a/src/style/GitStageStyle.ts b/src/style/GitStageStyle.ts
index 47c19a0..4da921b 100644
--- a/src/style/GitStageStyle.ts
+++ b/src/style/GitStageStyle.ts
@@ -54,7 +54,7 @@ export const changeStageButtonStyle = style({
backgroundColor: 'transparent',
backgroundPosition: 'center',
backgroundRepeat: 'no-repeat',
- backgroundSize: '14px',
+ backgroundSize: '20px',
transition: 'background-color 0.1s ease',
height: '13px',
width: '12px',
@@ -63,9 +63,9 @@ export const changeStageButtonStyle = style({
});
export const caretdownImageStyle = style({
- backgroundImage: 'var(--jp-image-caretdown)'
+ backgroundImage: 'var(--jp-icon-caret-down)'
});
export const caretrightImageStyle = style({
- backgroundImage: 'var(--jp-image-caretright)'
+ backgroundImage: 'var(--jp-icon-caret-right)'
});
diff --git a/src/style/PastCommitNode.ts b/src/style/PastCommitNode.ts
index a7929b9..4d31a0c 100644
--- a/src/style/PastCommitNode.ts
+++ b/src/style/PastCommitNode.ts
@@ -77,14 +77,14 @@ export const iconButtonClass = style({
});
export const expandIconButtonClass = style({
- backgroundImage: 'var(--jp-icon-caretdown)',
+ backgroundImage: 'var(--jp-icon-caret-down)',
backgroundSize: '20px',
backgroundRepeat: 'no-repeat',
backgroundPosition: 'center'
});
export const collapseIconButtonClass = style({
- backgroundImage: 'var(--jp-icon-caretup)',
+ backgroundImage: 'var(--jp-icon-caret-up)',
backgroundSize: '20px',
backgroundRepeat: 'no-repeat',
backgroundPosition: 'center'
diff --git a/src/style/Toolbar.ts b/src/style/Toolbar.ts
index 60bc463..7a1ddf6 100644
--- a/src/style/Toolbar.ts
+++ b/src/style/Toolbar.ts
@@ -153,14 +153,14 @@ export const branchIconClass = style({
});
export const openMenuIconClass = style({
- backgroundImage: 'var(--jp-icon-caretdown)',
+ backgroundImage: 'var(--jp-icon-caret-down)',
backgroundSize: '20px',
backgroundRepeat: 'no-repeat',
backgroundPosition: 'center'
});
export const closeMenuIconClass = style({
- backgroundImage: 'var(--jp-icon-caretup)',
+ backgroundImage: 'var(--jp-icon-caret-up)',
backgroundSize: '20px',
backgroundRepeat: 'no-repeat',
backgroundPosition: 'center'
diff --git a/style/diff-nb.css b/style/diff-nb.css
index 9ef379a..6780aa0 100644
--- a/style/diff-nb.css
+++ b/style/diff-nb.css
@@ -337,11 +337,11 @@
}
.jp-git-Notebook-diff .jp-CollapsiblePanel-header-icon-opened {
- background-image: var(--jp-icon-caretup);
+ background-image: var(--jp-icon-caret-up);
}
.jp-git-Notebook-diff .jp-CollapsiblePanel-header-icon-closed {
- background-image: var(--jp-icon-caretdown);
+ background-image: var(--jp-icon-caret-down);
}
.jp-git-Notebook-diff .jp-CollapsiblePanel-header {
5d84082
to
839f781
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @btel for pushing this forward!
I made a handful of small fixups and pushed them:
- rebased (again)
- one small tweak to a LabIcon call (I want to discourage the use of the base
LabIcon.resolve
method, there's a couple of ways you can shoot yourself in the foot with it) - updated yarn.lock
- I applied the diff Alex posted with the icon fixes. Thanks! Those helped
@btel After your make the changes @fcollonval requested I think this will be good to go.
Frederic, can you please open an issue laying out your plan for the release before this PR goes in?
@ajbozarth Good icon fixes! But... posting the contents of a raw patch file (that I then have to cut and paste) isn't the friendliest way to share your changes. In the future, don't be shy about just pushing minor fixes to the PR branch |
@telamonian I tried just pushing this changes but it said I didn't have permissions for some reason |
@ajbozarth Strange. I would have thought being a member of juptyerlab would have been permissions enough. Whelp, anyhow I just gave you write access for this repo specifically. Hopefully that means you'll be able to push to PRs in the future |
6404235
to
c91e898
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@fcollonval I am not sure what you were requesting (or maybe someone bit me to it) as you have admin access to the repo. If you were looking to update this pr, just create a checkout of the original branch from @btel and push to it (which is what I and @telamonian did). I have also created #602 for us to track the release process. I will have some time tomorrow afternoon to help with this. |
@lresende I gave @fcollonval admin rights |
Co-Authored-By: Frédéric Collonval <[email protected]>
Thanks all for comments/commits! @fcollonval I merged your suggestions (it took me a while before relising that I can do it directly from github UI ;-)) |
rebased and edited from #520
Fixes issue #559
Closes #520
For release schedule see #602