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

Restore padding on main dock panel #10390

Merged
merged 3 commits into from
Jun 14, 2021

Conversation

fcollonval
Copy link
Member

@fcollonval fcollonval commented Jun 11, 2021

References

Fixes #10387

Code changes

N/A

User-facing changes

Restore 5px padding on main dock panel

Backwards-incompatible changes

N/A

@jupyterlab-dev-mode
Copy link

Thanks for making a pull request to JupyterLab!

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

@github-actions github-actions bot added Design System CSS pkg:application tag:CSS For general CSS related issues and pecadilloes labels Jun 11, 2021
@fcollonval
Copy link
Member Author

fcollonval commented Jun 11, 2021

Before:

After:

Multiple document Single document
image image

Stable version

Credit for capture goes to @krassowski

UI tests are expected to fail.

@goanpeca
Copy link
Member

goanpeca commented Jun 11, 2021

ui-tests/reference-output/screenshots/general_dark_theme.png

Should we declare these files as binary (gitignore)to avoid having to diff and any conflicts in the future?

@fcollonval
Copy link
Member Author

Should we declare these files as binary (gitignore)to avoid having to diff and any conflicts in the future?

I'm not sure. If there are conflicts, this means they have been changed on master. So the PR needs to be rebased else UI tests wil fail when merging, no?

@krassowski
Copy link
Member

Diffing them via the GitHub interface is delightful by the way:

diff-github

I think we should continue reviewing them. Which brings me to a questions: @fcollonval do you happen to have ipykernel prerelease installed or was it picked up somewhere else?

@goanpeca
Copy link
Member

goanpeca commented Jun 11, 2021

I'm not sure. If there are conflicts, this means they have been changed on master. So the PR needs to be rebased else UI tests wil fail when merging, no?

I guess, but in general it does not seem like git should try to diff these files.

We can create a .gitattributes file and declare them as binary.

# 
*.png binary
*.PNG binary

Thoughts @jasongrout @mbektas @blink1073 ?


Update: Just missed @krassowski comment.

I guess I am not sure how the process is currently working for the UI tests, are we generating new images on every new run? (which would show what @krassowski is doing), cause I thought the new "reference" files had to be generated manually.

If we want to use the GIT diff, then we would need to generate these files on every PR so that we could check right away these differences, otherwise we are comparing between reference images, which can be useful 🤷🏼

@fcollonval
Copy link
Member Author

fcollonval commented Jun 11, 2021

do you happen to have ipykernel prerelease installed or was it picked up somewhere else?

Jeremy changed the CI to use the pre-release of ipykernel to test the debugger: https://github.com/jupyterlab/jupyterlab/pull/10359/files#r649835430

@jtpio
Copy link
Member

jtpio commented Jun 11, 2021

Yes updating to the ipykernel prerelease in that PR introduced a couple of visual changes.

From #10359 (comment):

image

@jtpio
Copy link
Member

jtpio commented Jun 11, 2021

I guess I am not sure how the process is currently working for the UI tests,

@goanpeca there are some instructions in https://github.com/jupyterlab/jupyterlab/tree/master/ui-tests and https://jupyterlab.readthedocs.io/en/latest/developer/contributing.html#visual-regression-and-ui-tests.

Usually we don't need to modify the reference screenshots unless there is an intentional visual change a PR wants to bring.

This PR fixes a regression that was not caught at the time because the visual regression tests were not in place yet.

@jtpio jtpio added this to the 3.1 milestone Jun 11, 2021
@fcollonval fcollonval force-pushed the fcollonval/issue10387 branch from c6baf8a to 037b006 Compare June 14, 2021 07:33
@fcollonval fcollonval force-pushed the fcollonval/issue10387 branch from 037b006 to 43fb1aa Compare June 14, 2021 07:35
@fcollonval fcollonval force-pushed the fcollonval/issue10387 branch from 01dfb82 to 1812941 Compare June 14, 2021 14:48
@fcollonval fcollonval marked this pull request as ready for review June 14, 2021 15:35
Copy link
Contributor

@blink1073 blink1073 left a comment

Choose a reason for hiding this comment

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

Thanks!

@blink1073 blink1073 merged commit c6b5e15 into jupyterlab:master Jun 14, 2021
@fcollonval fcollonval deleted the fcollonval/issue10387 branch June 14, 2021 16:11
@github-actions github-actions bot added the status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. label Dec 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Design System CSS maintenance pkg:application status:resolved-locked Closed issues are locked after 30 days inactivity. Please open a new issue for related discussion. tag:CSS For general CSS related issues and pecadilloes tag:Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Visual regression: border of the main area shrank
5 participants