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

Added hash to the csv delimiter types #7367

Merged
merged 1 commit into from Oct 15, 2019

Conversation

dkapila
Copy link
Contributor

@dkapila dkapila commented Oct 15, 2019

References

Fixes #6324

Code changes

Updated toolbar.js to add '#' as a possible combination.

User-facing changes

Users can select hash as a possible delimiter.

image

Backwards-incompatible changes

None.

@jupyterlab-dev-mode
Copy link

@jupyterlab-dev-mode jupyterlab-dev-mode bot commented Oct 15, 2019

Thanks for making a pull request to JupyterLab!

To try out this branch on binder, follow this link: Binder

jtpio
jtpio approved these changes Oct 15, 2019
Copy link
Member

@jtpio jtpio left a comment

Changes look good, thanks!

I agree this delimiter doesn't seem to be used often (as stated in #6324), but it can still be useful to have it available.

Leaving it open for a while in case someone has an objection.

@jtpio
Copy link
Member

@jtpio jtpio commented Oct 15, 2019

Test failure is not relevant (docs link check failing).

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 15, 2019

Thanks! How does this behave with the anchor references to a cell in a datagrid?

[some link in markdown](./test.csv#row=4)

(It shouldn't change anything, but just making sure...)

@dkapila
Copy link
Contributor Author

@dkapila dkapila commented Oct 15, 2019

@jasongrout

This is what we get:

image

What do you think the expected behaviour should be?

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 15, 2019

This is what we get:

Well, for one it shouldn't modify the file at all

Two: Can you try with a file that has at least 4 rows? What should happen is something like in the example in #5727

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 15, 2019

This is what we get:

Ah, just noticed your link is the same file. I meant:

  1. Create a test.csv file with hash delimiters, with at least 4 rows
  2. Create a markdown file next to it with the link [some link in markdown](./test.csv#row=4)
  3. Close the csv file
  4. Click the link - should open the file, but may be messed up because the delimiter is not set?
  5. Set the delimiter and click the link again
  6. Row 4 should be highlighted.

@dkapila
Copy link
Contributor Author

@dkapila dkapila commented Oct 15, 2019

@jasongrout

Thanks, I've tested the steps using hash, above and it works as expected.

ezgif-6-b1f9e8d43640

@jasongrout
Copy link
Contributor

@jasongrout jasongrout commented Oct 15, 2019

Thanks!

@jasongrout jasongrout added this to the 2.0 milestone Oct 15, 2019
@jasongrout jasongrout merged commit b5f6178 into jupyterlab:master Oct 15, 2019
7 of 9 checks passed
@jtpio
Copy link
Member

@jtpio jtpio commented Oct 15, 2019

Thanks @dkapila and congrats on your first contribution!

@dkapila dkapila deleted the feature-csv-hash branch Oct 15, 2019
@lock lock bot added the status:resolved-locked label Nov 14, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Nov 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pkg:csvviewer status:resolved-locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants