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 width and margins of the notebook footer. #16383

Conversation

HaudinFlorence
Copy link
Contributor

This PR fixes the width and the margins of the notebook footer.

References

This PR should fix issue #16380

Code changes

The proposes changes only concerns the style of the notebook footer and are in packages/notebook/style/notebookfooter.css

User-facing changes

Backwards-incompatible changes

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 pkg:notebook tag:CSS For general CSS related issues and pecadilloes Design System CSS labels May 28, 2024
@HaudinFlorence
Copy link
Contributor Author

bot please update snapshots

Copy link
Contributor

Documentation snapshots updated.

@krassowski krassowski added this to the 4.2.x milestone May 28, 2024
)
);
border: var(--jp-border-width) solid var(--jp-cell-editor-border-color);
border: 0.8px solid var(--jp-cell-editor-border-color);
Copy link
Member

Choose a reason for hiding this comment

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

What is the rationale for the change to 0.8px border?

Copy link
Contributor Author

@HaudinFlorence HaudinFlorence May 29, 2024

Choose a reason for hiding this comment

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

@krassowski Thanks for the review. I will revert this change. If I am not mistaking, I tried to fit the style of the notebook cell border I saw in the dev tools.

Copy link
Member

Choose a reason for hiding this comment

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

These snapshot changes appear unrelated and are causing the tests to fail I think:

Notice:   3 failed
    [documentation] › test/documentation/general.test.ts:618:7 › General › Command Palette ─────────
    [documentation] › test/documentation/internationalization.test.ts:14:7 › Internationalization › Menu 
    [documentation] › test/documentation/internationalization.test.ts:28:7 › Internationalization › Confirm language 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean that I should take back the snapshots from the main branch ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think that after that these tests should pass. If not, then we need to investigate (it looks like maybe different runners get different variants of Chinese fonts?)

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 for working on a fix! It looks good in the (now default) full windowing mode:

Screenshot from 2024-05-28 20-55-37

but in the none mode it seems that the spacing is too much:

Screenshot from 2024-05-28 20-55-27

@HaudinFlorence
Copy link
Contributor Author

Thank you for working on a fix! It looks good in the (now default) full windowing mode:

Screenshot from 2024-05-28 20-55-37

but in the none mode it seems that the spacing is too much:

Screenshot from 2024-05-28 20-55-27

Ok I was not aware of the none mode. I will try to fix that.

@HaudinFlorence
Copy link
Contributor Author

Thank you for working on a fix! It looks good in the (now default) full windowing mode:
Screenshot from 2024-05-28 20-55-37
but in the none mode it seems that the spacing is too much:
Screenshot from 2024-05-28 20-55-27

Ok I was not aware of the none mode. I will try to fix that.

After thinking a little bit about it, I don't know how to set different margin for the windowing mode and for the none mode.

@krassowski
Copy link
Member

@HaudinFlorence can you test the changes I pushed?

@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Jun 5, 2024

@HaudinFlorence can you test the changes I pushed?

@krassowski Thanks for working on this PR. In the none mode, the width and left margin are ok but the footer remains at its initial position when adding new cells:

Screencast.from.05-06-2024.17.26.31.webm

Note also that in both modes, the top margin of the footer is slightly too large

Also correct clearing of margin-right to leave no trace
@krassowski
Copy link
Member

Thank you for this recording! I saw this happen earlier on CI (#14385); I now realise what is happening - the button is positioned below the spacer jp-WindowedPanel-inner element, which has either:

  • managed height when full windowing mode is active
  • unset height when defer or none is used

I did not see the issue you pointer out in testing because I was reloading JupyterLab or opening a new notebook after changing settings. I think that you did not reload JupyterLab after the change and this revealed a bug in that the previously set height of jp-WindowedPanel-inner remained set after switching to none but it was no longer managed so it stuck on an incorrect value.

This should be fixed by aafff6e.

Note also that in both modes, the top margin of the footer is slightly too large

They seem fine to me. If there was any margin added explicitly I would accommodate your suggestion by reducing it, but since there is no explicit margin-top anymore I think maybe we can leave it as is?

@krassowski krassowski merged commit 6143d29 into jupyterlab:main Jun 7, 2024
81 of 82 checks passed
@krassowski
Copy link
Member

@meeseeksdev please backport to 4.2.x

meeseeksmachine pushed a commit to meeseeksmachine/jupyterlab that referenced this pull request Jun 7, 2024
krassowski pushed a commit that referenced this pull request Jun 7, 2024
…6453)

Co-authored-by: Florence Haudin <99649086+HaudinFlorence@users.noreply.github.com>
@HaudinFlorence
Copy link
Contributor Author

HaudinFlorence commented Jun 7, 2024

Thank you for this recording! I saw this happen earlier on CI (#14385); I now realise what is happening - the button is positioned below the spacer jp-WindowedPanel-inner element, which has either:

  • managed height when full windowing mode is active
  • unset height when defer or none is used

I did not see the issue you pointer out in testing because I was reloading JupyterLab or opening a new notebook after changing settings. I think that you did not reload JupyterLab after the change and this revealed a bug in that the previously set height of jp-WindowedPanel-inner remained set after switching to none but it was no longer managed so it stuck on an incorrect value.

This should be fixed by aafff6e.

Note also that in both modes, the top margin of the footer is slightly too large

They seem fine to me. If there was any margin added explicitly I would accommodate your suggestion by reducing it, but since there is no explicit margin-top anymore I think maybe we can leave it as is?

@krassowski Thanks for fixing the bug and completed this PR. We can keep the gap as it is now

@krassowski
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants