Sccarda/histogram style #2850
Sccarda/histogram style #2850
Conversation
ScottCarda-MS
requested review from
DmitryVasilevsky,
billti and
minestarks
as code owners
| --qs-histogram-menu-separator-stroke: #6b6b6b; | ||
| --qs-histogram-help-fill: #ffffff; | ||
| --qs-histogram-help-stroke: #6b6b6b; | ||
| --qs-histogram-help-text-fill: #202020; |
There was a problem hiding this comment.
Where did these color codes come from? Did you take them from any existing UX? We already are a little inconsistent across widgets, so I'd rather try and reuse colors from existing VS Code or QDK UX than add yet more divergence to our palette if possible.
There was a problem hiding this comment.
This was the color scheme that copilot suggested and I tweaked them a bit after until I thought they looked good. It is sort of difficult to find color combinations with three different colors that are all mutually visible against one another, which is needed here as the histogram bars have to be visible against the background, but the labels also need to be visible against both the bars and the background (for when they flow over the top of the bar graphic). My thinking is that we can play around with the color scheme a bit more to make things consistent across the extension if we really want to, but this color scheme makes everything visible and easily legible, which seems most important and is the motivation for the PR.
|
tested on vs code & jupyterlab on my machine, looks good to me. I'd prefer the Ket labels to be regular weight (instead of bold) though. |
source/widgets/js/index.tsx
Outdated
| // Set a stable color-scheme marker for CSS (works in Jupyter-only and VS Code). | ||
| // This is especially important when AnyWidget uses Shadow DOM, where selectors | ||
| // like body.jp-mod-theme-dark won't match inside the widget stylesheet. | ||
| applyAndObserveTheme(el); |
There was a problem hiding this comment.
I'm a little worried about this, and don't think it should go in this close to shipping without a lot more diligence and testing.
For example, the render method this call is in can be called at a very high frequence for the same widget (e.g. when the histogram animates such as when dragging a slider to change the noise and re-run, or when live updating with incoming shot results - basically any time Python wants to send new values to the widget to display).
Yet looking at this code it seems on every call to render you are doing quite a bit of work at the document level that could be quite expensive or cause a bit of document wide recalculating (e.g. if changing attributes of classes on the body node, as calling applyAndObserveTheme always calls apply, which always calls computeColorScheme and always calls setAttribute on the host. Bare in mind the containing document may be more than just a VS Code webview or Jupyter cell (e.g. we put the Histogram on the playground page, and there the 'document' is the entire playground).
As this is now global code that runs for every widget (not just histograms), I'd also want to ensure we do sufficient testing when multiple widgets are on the same "host", or even nested (as they are with some of the Resource Estimation views, which include the space chart, scatter chart, details table widgets all as children of the another container widget).
Bottom line: Maybe it's fine, but there's a lot of subtlety and hazzards here for some scenarios, so I think it should wait until after the Jan release. Thanks.
There was a problem hiding this comment.
Would it make sense to separate this logic, which is meant to be a fix for an existing bug for widgets in Jupyter, into a separate PR from the rest of the changes in this PR? That way we could have this PR focused on making stylistic changes for readability.
There was a problem hiding this comment.
Since this is a more involved fix for a pre-existing bug, I've moved the logic to fix that to a separate branch and kept this PR about updating the style. See https://github.com/microsoft/qdk/blob/sccarda/JupyterDarkThemeFix/source/widgets/js/index.tsx#L41 for the fix in the separate branch.
I've removed the bold. Thanks for the suggestion and for trying this out! |
To avoid difficult to read graphics, this PR removes the variability in the histogram colors as VS Code theme changes, and replaces it with two themes, a light theme histogram for light VS Code themes and a dark theme histogram for dark VS Code themes. Individual components should no longer change unless changing the VS Code theme between these two categories.
Light Themes:
Dark Themes:
Also improves the ket label font to be more legible.
Before:
After:
