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

Use Phosphor Datagrid for CSV Viewer #2433

Merged
merged 12 commits into from Jun 13, 2017
Merged

Conversation

@blink1073
Copy link
Member

@blink1073 blink1073 commented Jun 12, 2017

Removes the restriction on the number of renderable rows (from 1000 to the integer overflow value).

@blink1073 blink1073 added this to the Beta milestone Jun 12, 2017
@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Jun 12, 2017

For the record, integer "overflow" occurs at 9007199254740991.

I dunno, that may not be enough...

Loading

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Jun 12, 2017

You may consider setting .headerVisibility = 'column' to hide the row headers.

Loading

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Jun 12, 2017

Looks like the data format might be amenable to using the JSONModel instead of rolling your own.

Loading

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 12, 2017

Playing with it, I actually like seeing which row I'm on.

Loading

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 12, 2017

I looked into using JSONModel: we'd have to install a new one whenever the delimiter changes.

Loading

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 13, 2017

Removed the row number indicator to conform with what we had previously.

Loading

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Jun 13, 2017

I wonder if we should have a get/set data property on JSONModel then. Although, what's the use case for changing the delimiter in a CSV file. Shouldn't that be known up front?

Loading

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 13, 2017

Not necessarily, files can be called csv and use other delimiters.

Loading

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Jun 13, 2017

I meant for a given file, is there ever a need to change the delimiter?

Loading

@sccolbert
Copy link
Contributor

@sccolbert sccolbert commented Jun 13, 2017

Also, swapping data models on the grid is no more expensive than emitting model-reset.

Loading

@blink1073
Copy link
Member Author

@blink1073 blink1073 commented Jun 13, 2017

Switched to the JSONModel. The use case for the set-able delimiter in the toolbar is opening a file that you think is CSV only to realize it is tab or semi-colon separated.

Loading

afshin
afshin approved these changes Jun 13, 2017
@afshin afshin merged commit 47d3975 into jupyterlab:master Jun 13, 2017
1 of 2 checks passed
Loading
this._monitor = null;

model.dispose();
table.dispose();
grid.dispose();
Copy link
Contributor

@sccolbert sccolbert Jun 13, 2017

Choose a reason for hiding this comment

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

You don't need to dispose of your child widgets. The layout does it automatically.

Loading

@blink1073 blink1073 mentioned this pull request Jun 15, 2017
@blink1073 blink1073 deleted the phosphor-datagrid branch Jul 10, 2017
@lock lock bot locked as resolved and limited conversation to collaborators Aug 10, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants