Skip to content

Variable Explorer UI cleanup #5379

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 22, 2019
Merged

Variable Explorer UI cleanup #5379

merged 3 commits into from
Apr 22, 2019

Conversation

@IanMatthewHuff
Copy link
Member

For #5274

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR)
  • Title summarizes what is changing
  • Has a news entry file (remember to thank yourself!)
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated
  • Test plan is updated as appropriate
  • package-lock.json has been regenerated by running npm install (if dependencies have changed)
  • The wiki is updated with any design decisions/details.
@IanMatthewHuff IanMatthewHuff requested a review from rchiodo April 19, 2019 18:08
@IanMatthewHuff
Copy link
Member Author

Cleaned up ->

Double click a row to open data explorer
Use editor font for var exp rows
More spacing around variable expander and data grid
Show variable name in Data Viewer
DonJayamanne
DonJayamanne approved these changes Apr 19, 2019
View reviewed changes
src/client/datascience/data-viewing/dataViewer.ts
let newTitle = `${localize.DataScience.dataExplorerTitle()} - ${variable.name}`;
const TRIM_LENGTH = 40;
if (newTitle.length > TRIM_LENGTH) {
newTitle = `${newTitle.substr(0, TRIM_LENGTH)}...`;
Copy link

Choose a reason for hiding this comment

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

I would have expected such truncation to take place in CSS based on relative widths (e.g. %s of the total width, along with min width.. or similar)
Copy link
Member Author

Choose a reason for hiding this comment

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

@DonJayamanne Hmmm, do you know of how we could actually see the widths? This is the string that goes in the VSCode editor file tab. We don't own the UI for that in our extension. When testing I realized that it doesn't truncate in the vscode editor tab (at least I was able to make a tab that took up basically my entire editor tab space) so this was just a sanity check to avoid a degenerate case with a super long variable name.
Copy link

Choose a reason for hiding this comment

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

Oh, if its for VSC, then its fine. I thought this was for the react webview.
rchiodo
rchiodo approved these changes Apr 19, 2019
View reviewed changes
Copy link

@rchiodo rchiodo left a comment

Choose a reason for hiding this comment

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

:shipit:
@IanMatthewHuff IanMatthewHuff merged commit 6f0697a into microsoft:master Apr 22, 2019
IanMatthewHuff added a commit that referenced this pull request Apr 22, 2019
@IanMatthewHuff
Variable Explorer UI cleanup (#5379)
149bf8b 
* variable explorer UI feedback fixes * add new entry * review feedback
@IanMatthewHuff IanMatthewHuff deleted the dev/ianhu/dataExplorerIcon branch July 25, 2019 23:50
@lock lock bot locked as resolved and limited conversation to collaborators Aug 1, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

3 participants

@IanMatthewHuff @DonJayamanne @rchiodo