-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Styling fixes for rjsm Settings Editor: remove accordions, rework "Restore to defaults" button, add placeholder #14074
Styling fixes for rjsm Settings Editor: remove accordions, rework "Restore to defaults" button, add placeholder #14074
Conversation
Thanks for making a pull request to jupyterlab! |
edd056b
to
7a0cca2
Compare
bot please update snapshots |
Alternative version in #14073 with descriptions present. I think it looks more bunched up while not being more informative: |
Benchmark reportThe execution time (in milliseconds) are grouped by test file, test type and browser. The mean relative comparison is computed with 95% confidence. Results table
Changes are computed with expected as reference.
Waiting for localhost:8888 Cell memory leaksCreate a code cellMemory change: +150 kB Leak detected: YesLeaking objects:
Leaking collections: Create a markdown cellMemory change: -105 kB Leak detected: NoLeaking objects:
Leaking collections: Create a raw cellMemory change: +140 kB Leak detected: YesLeaking objects:
Leaking collections:
File editor memory leaksCreate a fileMemory change: -75.2 kB Leak detected: NoLeaking objects:
Notebook memory leaksCreate a notebookMemory change: +23 kB Leak detected: YesLeaking objects:
2 passing (6m)
|
Galata snapshots updated. |
Kicking CI |
@marthacryan, requesting your review as from author of existing Settings Editor accordion header styling. @fcollonval, @krassowski, @isabela-pf requesting your review as I thought that you might have thoughts / opinions on this proposed change. Would also mention this PR during JupyterLab call on Wednesday. |
bot please update documentation snapshots |
Documentation snapshots updated. |
I do have an opinion, but not necessarily one that everyone agrees with: In my opinion we should removal all the accordions as these serve no useful purpose when each section takes up full screen. Instead we should only ever show the active section (the same way as JSON Settings Editor does works - one section ever visible at a time) UNLESS in search mode (where we do not show titles. As a user I never found myself in a need of having two sections of settings open simultaneously and was always annoyed by the accordion because it detracts attention. If that was to be the solution we decide to pursue removing description may not be necessary. I think I brought up this before late in the process of merging the original UI in so I am not sure if this is something that folks do not agree with or just did not have a chance to think about. |
I agree with @krassowski that removing Accordions altogether would be a good idea. The only additional functionality that accordions provide vs settings menu to the left is quick access to "Restore to Defaults" button (if it exists, majority of accordions don't have it), otherwise accordions duplicate menus functionality. Accordions might play some role that I'm missing. I looked at PR #11079 that adds accordion headers and issue #8148 that PR references as a design spec but they don't mention accordions. |
bot please update documentation snapshots |
2c8b289
to
f03011e
Compare
@krassowski @fcollonval It's all green now 🟢 |
@fcollonval @krassowski @marthacryan, for future reference, when scenario tested in I see that |
@@ -320,18 +277,34 @@ | |||
height: 100%; | |||
} | |||
|
|||
.jp-SettingsForm .jp-Buttonbar { |
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.
Let's discuss whether we want to have button bar in #14403. on CSS-level, I would suggest to replace jp-Buttonbar
with a more specific jp-SettingsForm-buttonbar
to avoid introducing a new unscoped class which is only used in a single place (and for performance reasons) - but this can be done in a separate PR
justify-content: flex-end; | ||
margin: 8px; | ||
gap: 0.5em; | ||
max-width: 800px; |
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.
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 left suggestions for improvements in #14403; I think that if we want this in for 4.0 we should merge as is and iterate on style details.
Thank you @krassowski. I copied your latest per-line suggestions into #14403 so we could continue discussing them. |
|
Documentation snapshots updated. |
Kicking CI |
After updating documentation snapshots, Documentation visual regression test doesn't fail anymore but |
Galata snapshots updated. |
Kicking CI |
Failing |
212f6e7
to
649faec
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.
Thanks @andrii-i
Thank you @fcollonval |
References
Code changes
Remove accordions from Settings Editor, rework "Restore to defaults" button to be shown as a part of "button bar" area, add placeholder for when no plugin is chosen from pluginlist to rjsm, reuse it for JSON editor as well.
On search, stack of plugins with matching fields would be shown (in line with for example VSCode).
User-facing changes
Current head-of-master:
With this change:
Backwards-incompatible changes
None