Restyle data viewer and add slice data panel #4805

joyceerhl · 2021-02-13T03:20:44Z

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!).
Appropriate comments and documentation strings in the code.
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).

codecov-io · 2021-02-13T04:15:08Z

Codecov Report

Merging #4805 (1c582ed) into main (b0ac45b) will increase coverage by 2%.
The diff coverage is 60%.

@@          Coverage Diff           @@
##            main   #4805    +/-   ##
======================================
+ Coverage     73%     76%    +2%     
======================================
  Files        407     401     -6     
  Lines      26816   26583   -233     
  Branches    3874    3854    -20     
======================================
+ Hits       19800   20306   +506     
+ Misses      5411    4671   -740     
- Partials    1605    1606     +1

Impacted Files	Coverage Δ
src/client/common/application/types.ts	`100% <ø> (ø)`
...cience/data-viewing/jupyterVariableDataProvider.ts	`68% <ø> (ø)`
src/client/datascience/data-viewing/types.ts	`100% <ø> (ø)`
src/client/datascience/types.ts	`100% <ø> (ø)`
...t/common/application/webviewPanels/webviewPanel.ts	`69% <33%> (-4%)`	⬇️
...rc/client/datascience/jupyter/debuggerVariables.ts	`79% <40%> (-2%)`	⬇️
src/client/datascience/data-viewing/dataViewer.ts	`69% <70%> (-2%)`	⬇️
src/client/datascience/jupyter/kernelVariables.ts	`73% <100%> (+<1%)`	⬆️
src/client/datascience/raw-kernel/rawSession.ts	`53% <0%> (-4%)`	⬇️
...lient/common/application/applicationEnvironment.ts	`52% <0%> (-2%)`	⬇️
... and 90 more

…microsoft/vscode-jupyter into joyceerhl/fix-dataviewer-styles

…to joyceerhl/fix-dataviewer-styles

joyceerhl · 2021-02-16T21:17:41Z

Should probably give a courtesy heads up to the AML extension team that we're changing the dataset preview UI.

…to joyceerhl/fix-dataviewer-styles

rchiodo · 2021-02-22T21:40:56Z

src/datascience-ui/react-common/seti/seti.less

@@ -0,0 +1,656 @@
+@font-face {


Did you copy this from somewhere? If so we need to add a license for it to the ThirdPartyRespository.txt

It's the seti icons that VS Code ships. Needed this for the language icon in the breadcrumbs at the top of the data viewer. This is the one that VS Code has: https://github.com/microsoft/vscode/blob/main/extensions/theme-seti/ThirdPartyNotices.txt

Do we add the license manually, or is there a component governance autogeneration step somewhere? And do we need to add it to both ThirdPartyNotices-Distribution as well as ThirdPartyNotices-Repository?

Updated Repository notice and manually added this dependency to AzDO CG

rchiodo · 2021-02-22T21:41:29Z

package.json

@@ -1914,6 +1914,7 @@
    },
    "dependencies": {
        "@enonic/fnv-plus": "^1.3.0",
+        "@fluentui/react": "^7.160.1",


Are you actually using this? I thought you dropped it?

I tried to use this wherever I could, so yes I am using this in places where it was easier to reuse + override styles than it was to write my own.

yay for using fluentui/react.... its awesome..

DonJayamanne · 2021-02-22T23:06:51Z

src/client/datascience/data-viewing/types.ts

@@ -68,6 +68,8 @@ export interface IDataFrameInfo {
    maximumRowChunkSize?: number;
    type?: string;
    originalVariableType?: string;
+    name?: string;
+    fileName?: string;


Would be nice to have come comments. What's FileName here?
The rest seem to make sense, as they are associated with a variable.

DonJayamanne · 2021-02-22T23:08:53Z

src/client/datascience/jupyter/debuggerVariables.ts

        // Results should be the updated variable.
        return results
            ? {
                  ...targetVariable,
                  ...JSON.parse(results.result),
-                  maximumRowChunkSize: MaximumRowChunkSizeForDebugger
+                  maximumRowChunkSize: MaximumRowChunkSizeForDebugger,
+                  fileName


Isn't this the name of the file where the frame is. Not sure fileName is meaningful enough.
Initially i thought it could be the file path for a data frame (source where it was loaded from).
After looking at this code I now konw this is the file path from the frame.

I.e. i'd consider re-naming the property (still not clear why we need this & why its associated with the variable - hopefully i'll see this being used later in the code).

src/datascience-ui/data-explorer/mainPanel.tsx

DonJayamanne · 2021-02-22T23:15:04Z

src/datascience-ui/data-explorer/mainPanel.tsx

@@ -233,6 +264,8 @@ export class MainPanel extends React.Component<IMainPanelProps, IMainPanelState>
                const indexColumn = variable.indexColumn ? variable.indexColumn : 'index';
                const originalVariableType = this.state.originalVariableType ?? variable.type;
                const originalVariableShape = this.state.originalVariableShape ?? variable.shape;
+                const variableName = this.state.variableName ?? variable.name;
+                const fileName = this.state.fileName ?? variable.fileName;


Yeah, its weird for a variable to have a file name. I'd prefer this to be sent as a separate entity.

Is it really that weird though? The variable must've been declared somewhere. I think it should definitely be renamed but it doesn't seem that weird to me to have backingFileName or similar as an attribute associated with the variable info.

It is weird, if you look at the debugger prototcol or others, variables are not attached to a file name. We have a file name that can contains a context & that context then contains a list of variables & that list contains the individual variable.
I.e. the mapping is at a much higher level & this seem wrong to me.
But don't want to block this PR for that.

I.e. the request object (sending request to kernel for variables) should already know the file name that is active.
Also if we're displaying variables for Notebooks, would we display the file name as some internal python file or the name of the notebook? Today run by line works in webview, if we use run by line & display the variables, what do you see in the file name (breadcrumb)?

Not worth blocking PR

But i'd test this with run by line & the others, not sure the name of the file would be as expected/required. I.e. in notebooks I'd always expect to see the name of the notebook if anything.

The request we send to the kernel doesn't ask for the filename. The filename property is only included on the variable at the point when we send the variable info to the UI for display.

For webview notebooks the file name is the name of the notebook, e.g.
. This is taken from notebook.identity.path.

For RBL I initially went with debugLocation because notebook.identity is not available when debugging a Python file, but this doesn't work for RBL, the filename would've been '<ipython input>'. Changed debuggerVariables.ts to use the notebook.identity.path when it's available (which it is for Run By Line), and use the debugLocation when it's not (i.e. debugging a Python file). Good catch :)

DonJayamanne · 2021-02-22T23:18:18Z

src/datascience-ui/data-explorer/reactSlickGridFilterBox.tsx

@@ -3,11 +3,26 @@
 'use strict';

 import * as React from 'react';
+import { IIconProps, initializeIcons, SearchBox } from '@fluentui/react';
+
+initializeIcons();


I thought this needs to be done one place, if thats the case then I'd move it to the entry point of our UI (so that we dont' have multiples of these)

Moved to index.tsx, thanks!

src/datascience-ui/data-explorer/reactSlickGridFilterBox.tsx

src/test/datascience/mountedWebView.ts

DonJayamanne

Everything looks ok, except for the fileName, not sure it should be part of the variable entity.
If anything I'd try to create a separate object that contains those two such as or similarl/better...

type StackVariable = {
	variabel: IDataVariable;
	stackFileName: string;
}

…to joyceerhl/fix-dataviewer-styles

joyceerhl · 2021-02-23T22:02:40Z

Made a couple more changes and fixes. Main difference is we are now generating a numeric (as opposed to guid) index column in the UI (as opposed to in our Python scripts) for all data, even if the data already contains an index column. This is to avoid breaking AML with the new filter button changes. AML preview datasets now look like this:

joyceerhl added 3 commits February 12, 2021 16:15

Improve cell and input styles

73620a3

New filter box

15e83af

New filter box

0b0eaad

joyceerhl changed the title ~~Restyle data viewer~~ Restyle data viewer filtering and cells Feb 13, 2021

joyceerhl added 4 commits February 15, 2021 21:36

Remove filter button

ad38fd6

Merge branch 'joyceerhl/fix-dataviewer-styles' of https://github.com/…

329cbe5

…microsoft/vscode-jupyter into joyceerhl/fix-dataviewer-styles

Linter

f581653

Merge branch 'main' of https://github.com/microsoft/vscode-jupyter in…

d020472

…to joyceerhl/fix-dataviewer-styles

joyceerhl changed the title ~~Restyle data viewer filtering and cells~~ Restyle data viewer filtering and cells and remove filter button Feb 16, 2021

joyceerhl added 2 commits February 16, 2021 12:25

Merge branch 'main' of https://github.com/microsoft/vscode-jupyter in…

2419536

…to joyceerhl/fix-dataviewer-styles

Lint + format

31651d0

joyceerhl added 13 commits February 18, 2021 15:46

Add breadcrumbs

ee3dd02

Dropdowns WIP

1cb1b73

Distinguish input value from applied slice

e0a7be8

Fix input to dropdown sync for 2D case

7a67279

Remember preferred viewgroup for data viewer

924516e

Fix if data doesn't support slicing

5c9f31f

Some cleanup

21abc05

More cleanup

ce8dc01

Fix linter

049233f

Merge branch 'main' of https://github.com/microsoft/vscode-jupyter in…

713fd22

…to joyceerhl/fix-dataviewer-styles

Validate index errors in slice expression

045b1b1

Make most methods private

1b1971d

New index column header buttons for clear and filter

5970c68

joyceerhl changed the title ~~Restyle data viewer filtering and cells and remove filter button~~ Restyle data viewer and add slice data panel Feb 22, 2021

joyceerhl marked this pull request as ready for review February 22, 2021 19:30

joyceerhl requested a review from a team as a code owner February 22, 2021 19:30

Prettier

a956ee9

rchiodo reviewed Feb 22, 2021

View reviewed changes

rchiodo approved these changes Feb 22, 2021

View reviewed changes

DonJayamanne reviewed Feb 22, 2021

View reviewed changes

src/datascience-ui/data-explorer/mainPanel.tsx Show resolved Hide resolved

DonJayamanne reviewed Feb 22, 2021

View reviewed changes

src/datascience-ui/data-explorer/mainPanel.tsx Show resolved Hide resolved

DonJayamanne reviewed Feb 22, 2021

View reviewed changes

src/datascience-ui/data-explorer/reactSlickGridFilterBox.tsx Show resolved Hide resolved

DonJayamanne reviewed Feb 22, 2021

View reviewed changes

src/test/datascience/mountedWebView.ts Show resolved Hide resolved

DonJayamanne requested changes Feb 22, 2021

View reviewed changes

joyceerhl added 2 commits February 22, 2021 22:14

Style fixes, bug fix, add comment

c58f8b8

Merge branch 'main' of https://github.com/microsoft/vscode-jupyter in…

1a0d18e

…to joyceerhl/fix-dataviewer-styles

DonJayamanne approved these changes Feb 23, 2021

View reviewed changes

joyceerhl added 10 commits February 23, 2021 10:17

Use notebook.identity.path for breadcrumb file name where available

caf57ce

Update ThirdPartyNotices-Repository

c15a964

Generate index column in UI

1e10952

Don't render slice panel for 1D data

f823e4e

Replace guid with index beginning at 0

c44936c

Unset dropdowns on range slices

1c582ed

fluentui initializeIcons in index.tsx

0345d6e

Better error message for index errors

281aa6e

Update eslintrc.js

7e34db2

Oops

184daae

joyceerhl merged commit b634a4d into main Feb 23, 2021

joyceerhl deleted the joyceerhl/fix-dataviewer-styles branch February 23, 2021 22:31

This was referenced Feb 25, 2021

DataFrame viewer keeps opening on top of the notebook #4689

Closed

Data viewer should clearly indicate when data is filtered #4462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restyle data viewer and add slice data panel #4805

Restyle data viewer and add slice data panel #4805

joyceerhl commented Feb 13, 2021 •

edited

Loading

codecov-io commented Feb 13, 2021 •

edited

Loading

joyceerhl commented Feb 16, 2021

rchiodo Feb 22, 2021

joyceerhl Feb 23, 2021

joyceerhl Feb 23, 2021

joyceerhl Feb 23, 2021

rchiodo Feb 22, 2021

joyceerhl Feb 22, 2021

DonJayamanne Feb 22, 2021

DonJayamanne Feb 22, 2021

DonJayamanne Feb 22, 2021

DonJayamanne Feb 22, 2021

DonJayamanne Feb 22, 2021

joyceerhl Feb 23, 2021

DonJayamanne Feb 23, 2021

DonJayamanne Feb 23, 2021

DonJayamanne Feb 23, 2021

joyceerhl Feb 23, 2021 •

edited

Loading

DonJayamanne Feb 22, 2021

joyceerhl Feb 23, 2021

DonJayamanne left a comment

joyceerhl commented Feb 23, 2021

Restyle data viewer and add slice data panel #4805

Restyle data viewer and add slice data panel #4805

Conversation

joyceerhl commented Feb 13, 2021 • edited Loading

codecov-io commented Feb 13, 2021 • edited Loading

Codecov Report

joyceerhl commented Feb 16, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joyceerhl Feb 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DonJayamanne left a comment

Choose a reason for hiding this comment

joyceerhl commented Feb 23, 2021

joyceerhl commented Feb 13, 2021 •

edited

Loading

codecov-io commented Feb 13, 2021 •

edited

Loading

joyceerhl Feb 23, 2021 •

edited

Loading