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

More data viewer style tweaks #5055

Merged
merged 7 commits into from
Mar 8, 2021
Merged

More data viewer style tweaks #5055

merged 7 commits into from
Mar 8, 2021

Conversation

joyceerhl
Copy link
Contributor

Found a few issues while testing with different themes, plus some more tweaks to make the data viewer look like the figma mockups.

image

@joyceerhl joyceerhl requested a review from a team as a code owner March 6, 2021 20:43
id="slice-enablement-checkbox"
className="slice-enablement-checkbox"
<Checkbox
styles={checkboxStyles}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Figured out how to style FluentUI controls without hacks, went back and changed this to a FluentUI checkbox

@codecov-io
Copy link

codecov-io commented Mar 6, 2021

Codecov Report

Merging #5055 (efb4728) into main (2e44ccb) will decrease coverage by 0%.
The diff coverage is n/a.

@@          Coverage Diff          @@
##            main   #5055   +/-   ##
=====================================
- Coverage     76%     76%   -1%     
=====================================
  Files        404     404           
  Lines      26877   26877           
  Branches    3894    3894           
=====================================
- Hits       20495   20481   -14     
- Misses      4759    4778   +19     
+ Partials    1623    1618    -5     
Impacted Files Coverage Δ
...t/datascience/jupyter/jupyterInvalidKernelError.ts 66% <0%> (-34%) ⬇️
...t/datascience/notebook/helpers/executionHelpers.ts 67% <0%> (-10%) ⬇️
src/client/datascience/jupyter/jupyterSession.ts 75% <0%> (-8%) ⬇️
...science/jupyter/kernels/kernelDependencyService.ts 84% <0%> (-6%) ⬇️
...lient/datascience/jupyter/kernels/cellExecution.ts 77% <0%> (-5%) ⬇️
src/client/common/process/baseDaemon.ts 54% <0%> (-3%) ⬇️
src/client/datascience/baseJupyterSession.ts 74% <0%> (-2%) ⬇️
src/client/datascience/jupyter/notebookStarter.ts 77% <0%> (-2%) ⬇️
src/client/datascience/jupyter/kernels/helpers.ts 64% <0%> (-1%) ⬇️
...datascience/notebookStorage/nativeEditorStorage.ts 80% <0%> (-1%) ⬇️
... and 12 more

case ColumnType.Number:
return this.renderNumber(this.props.value as number);

default:
break;
}
}
// Otherwise an unknown type or a string
// Otherwise an unknown type, boolean, or a string
Copy link
Member

Choose a reason for hiding this comment

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

Yay for updating comments. thanks.

text-align: right;
display: flex;
padding: 3px;
color: var(--vscode-settings-textInputForeground);
Copy link
Member

Choose a reason for hiding this comment

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

Bit hard to track through all the CSS, but this color looks a bit suspicious to me. It's from the GUI settings editor specifically.
https://code.visualstudio.com/api/references/theme-color#settings-editor-colors
I would expect it only to be used alongside textInputBackground which I'm not currently seeing. Using it against other background could lead to accessibility issues.

Copy link
Member

Choose a reason for hiding this comment

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

Whoops, looks like the background is added later in this PR. This should be ok then as long at this icon is showing up under .slice-data. Is that the case?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm this is actually used against --vscode-menu-background and it's the token used in the figma mockups

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Switching to menu-foreground after discussing with Jill. Eventually I think we want to use whatever the new table widget is using, but I'll wait till the design for that has stabilized.

Copy link
Member

Choose a reason for hiding this comment

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

Awesome. Thanks for following up with Jill. These color usage mismatches used to cause lots of issue back in Visual Studio so I try to push being strict on them as much as possible.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did another pass and found a few more token mismatches for foreground vs background, so fixed those too :)

Copy link
Member

@IanMatthewHuff IanMatthewHuff left a comment

Choose a reason for hiding this comment

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

Approved as long as everything using textInputForeground is under textInputBackground elements.

@joyceerhl joyceerhl merged commit 4ff0c31 into main Mar 8, 2021
@joyceerhl joyceerhl deleted the joyceerhl/more-styles branch March 8, 2021 22:53
joyceerhl added a commit that referenced this pull request Mar 23, 2021
joyceerhl added a commit that referenced this pull request Mar 24, 2021
* More data viewer style tweaks (#5055)

* Add slice data functional tests (#5057)

* Changelog and news entry

* Fix smoke tests

* Lint

Co-authored-by: Don Jayamanne <don.jayamanne@yahoo.com>
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.

5 participants