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

Fix editor background color #714

Merged
merged 7 commits into from
Oct 25, 2023

Conversation

HaudinFlorence
Copy link
Contributor

@HaudinFlorence HaudinFlorence commented Oct 16, 2023

This PR aims at fixing the missing background color for the codemirror editors, to be aligned with jupyterlab cells' color (when unselected). It adresses the case of the webapp. The background is not yet fixed for the labextension.

The re-introduced background color is the one associated with the css variable --jp-cell-editor-background as can be seen on the following capture:

Screenshot from 2023-10-16 17-46-55

Copy link
Collaborator

@fcollonval fcollonval left a comment

Choose a reason for hiding this comment

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

@krassowski
Copy link
Member

I have two questions here:

The background is not yet fixed for the labextension.

  1. In that case should we replace the "fixes" keyword so that the issue CM6: Fix background color #675 does not get closed on merging this PR?

  2. Why are there no changes to the snapshots? Is it not covered by snapshots, or is it that the snapshots do differ but the change in colour is below sensitivity threshold and did not get picked up? If it is the latter, shall we tighten the threshold to ensure we catch visual regressions in the bg color?

@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Oct 21, 2023

@krassowski Thanks for having a look at this PR.

I have two questions here:

The background is not yet fixed for the labextension.

  1. In that case should we replace the "fixes" keyword so that the issue CM6: Fix background color #675 does not get closed on merging this PR?

In fact, locally the change was not visible in my labextension. Could you please check on your local one? Normally the backgorund should be taken into account because the changes are introduced in nbdime/lib/styles/common.css that is imported in labextension/style/index.css. (For the moment I have removed the fixes mention as you proposed).

  1. Why are there no changes to the snapshots? Is it not covered by snapshots, or is it that the snapshots do differ but the change in colour is below sensitivity threshold and did not get picked up? If it is the latter, shall we tighten the threshold to ensure we catch visual regressions in the bg color?

I think it was only a matter of updating the snapshots.

@krassowski
Copy link
Member

I think it was only a matter on updating the snapshots.

Well, if the tests were all green before updating the snapshots it strongly suggests that we should adjust maxDiffPixelRatio or maxDiffPixels to catch a potential regression in the future.

Reference: https://playwright.dev/docs/api/class-pageassertions#page-assertions-to-have-screenshot-1

@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

@HaudinFlorence
Copy link
Contributor Author

I think it was only a matter on updating the snapshots.

Well, if the tests were all green before updating the snapshots it strongly suggests that we should adjust maxDiffPixelRatio or maxDiffPixels to catch a potential regression in the future.

Reference: https://playwright.dev/docs/api/class-pageassertions#page-assertions-to-have-screenshot-1

I am not completely sure that all tests were green. I take note about your suggestion.

@krassowski
Copy link
Member

The windows snapshots seem to have been inadvertly deleted.

bot please update playwright snapshots

@github-actions
Copy link
Contributor

Playwright ubuntu-22.04 snapshots updated.

@github-actions
Copy link
Contributor

Playwright windows-latest snapshots updated.

@fcollonval
Copy link
Collaborator

Triggering the CI with the latest snapshots

@fcollonval fcollonval closed this Oct 25, 2023
@fcollonval fcollonval reopened this Oct 25, 2023
@krassowski
Copy link
Member

Good call, failed in tests.

bot please update playwright snapshots

@github-actions
Copy link
Contributor

Playwright ubuntu-22.04 snapshots updated.

Copy link
Member

@krassowski krassowski left a comment

Choose a reason for hiding this comment

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

Thank you @HaudinFlorence, snapshots look good now.

@krassowski krassowski merged commit 5efc481 into jupyter:master Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants