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

CSV editor #911

Merged
merged 18 commits into from Jan 25, 2016

Conversation

Projects
None yet
3 participants
@timwis
Contributor

timwis commented Jan 2, 2016

screen shot 2016-01-02 at 18 01 50

Adds a spreadsheet editor for CSV files using handsontable and PapaParse.

It's fully functional but I'd still call it a work in progress as I hope to solicit your feedback before recommending a merge. The following items remain:

  • CommonJS dependency (Requiring handsontable is tricky because it's written in ES2015. Right now it's pulling in the built, minified file)
  • Get clarity: I wasn't sure the best way to pull in handsontable's CSS - it seems like the only CSS in prose is in styles.css - are there no CSS dependencies?
  • It should work for TSV files too (PapaParse supports it)
  • Auto-height (I don't like that it has a fixed height of 600px, but in order to get smooth overflow scrolling, handsontable needs a default height. We can compute this based on available vertical real estate, I imagine.)
  • Undo/redo support
  • Add/Delete row/column support
  • Column resize support
  • Compare mobile support to standard prose mobile support and ensure it aligns
  • Bug: Unsaved changes don't show up as 'dirty' when you return to the file
  • Add ability to switch to plain file editor
  • Get travis to pass
  • Add unit tests

I welcome your thoughts and feedback!

EDIT: Looks like legacy node builds in travis are failing because handsontable is installed via github (Unsupported URL Type: github:handsontable/handsontable). I'm sure there's an easy fix.

@phillipadsmith

This comment has been minimized.

Show comment
Hide comment
@phillipadsmith

phillipadsmith Jan 4, 2016

Member

Hey there @timwis --

I'm just shaking off a holiday haze.

At first glance, this looks like an impressive potential addition to Prose.

And yet, there are probably a couple of issues that need attention before it can be considered:

  1. Are you going to address the issue in package.json that's causing the test failure?
  2. Are you thinking about adding some tests for these features?

I'll aim to get this installed locally and have a closer look this week, unless @dereklieu beats me to it (though, I'm guessing he's shorter on time than I am).

Phillip.

Member

phillipadsmith commented Jan 4, 2016

Hey there @timwis --

I'm just shaking off a holiday haze.

At first glance, this looks like an impressive potential addition to Prose.

And yet, there are probably a couple of issues that need attention before it can be considered:

  1. Are you going to address the issue in package.json that's causing the test failure?
  2. Are you thinking about adding some tests for these features?

I'll aim to get this installed locally and have a closer look this week, unless @dereklieu beats me to it (though, I'm guessing he's shorter on time than I am).

Phillip.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 4, 2016

Contributor

Thanks @phillipadsmith! I'm definitely up for both those items along with the other points I made above. I just wanted to get your high-level feedback before diving any deeper to make sure I'm approaching it the right way and that this would be a valued feature.

Contributor

timwis commented Jan 4, 2016

Thanks @phillipadsmith! I'm definitely up for both those items along with the other points I made above. I just wanted to get your high-level feedback before diving any deeper to make sure I'm approaching it the right way and that this would be a valued feature.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 5, 2016

Contributor

FYI, travis passing and TSV support added. Still a few other items above, but progress :)

Contributor

timwis commented Jan 5, 2016

FYI, travis passing and TSV support added. Still a few other items above, but progress :)

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 5, 2016

Contributor

At this point I'd like to get your thoughts on using handsontable from a CDN rather than compiling it into the prose.js build. It's pretty difficult to get it to build properly since it's written in ES2015 with their own compiler. They offer a compiled-to-ES5 version that can work standalone, but browserify tries to replace all the require()s with its own require logic, and I believe that's what's throwing errors.

Contributor

timwis commented Jan 5, 2016

At this point I'd like to get your thoughts on using handsontable from a CDN rather than compiling it into the prose.js build. It's pretty difficult to get it to build properly since it's written in ES2015 with their own compiler. They offer a compiled-to-ES5 version that can work standalone, but browserify tries to replace all the require()s with its own require logic, and I believe that's what's throwing errors.

@dereklieu

This comment has been minimized.

Show comment
Hide comment
@dereklieu

dereklieu Jan 5, 2016

Member

@timwis that would be easier I agree, but since this will come up as more and more libraries adopt ES2015, I would rather add something like a babel transform to package.json. Would that solve this issue?

Member

dereklieu commented Jan 5, 2016

@timwis that would be easier I agree, but since this will come up as more and more libraries adopt ES2015, I would rather add something like a babel transform to package.json. Would that solve this issue?

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 5, 2016

Contributor

@dereklieu it may...we'd have to configure babelify to only transpile this module. Though I tried exactly this with webpack a couple weeks ago and it didn't work because the handsontable team used some other, non-babel transpiler and the local dependencies weren't pulled in (I assume when they rewrote in ES2015, babel wasn't as trendy). I'll give it a shot though.

Contributor

timwis commented Jan 5, 2016

@dereklieu it may...we'd have to configure babelify to only transpile this module. Though I tried exactly this with webpack a couple weeks ago and it didn't work because the handsontable team used some other, non-babel transpiler and the local dependencies weren't pulled in (I assume when they rewrote in ES2015, babel wasn't as trendy). I'll give it a shot though.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 5, 2016

Contributor

Also, mobile support is weak for handsontable -- it seems like it works but gets cut off after a certain number of rows. Is that a deal breaker?

Contributor

timwis commented Jan 5, 2016

Also, mobile support is weak for handsontable -- it seems like it works but gets cut off after a certain number of rows. Is that a deal breaker?

@dereklieu

This comment has been minimized.

Show comment
Hide comment
@dereklieu

dereklieu Jan 5, 2016

Member

Thanks for flagging mobile support. I'm a bit conflicted here between maintaining mobile support for the default Safari that comes on lots of iOS devices. Not sure from their docs what Android support looks like.

Thinking about user expectations, I would see this as an extension of core functionality, so if we left that functionality intact while providing simple fallback for unsupported devices, I think I'd be ok with that.

I was hoping to find some info on the handsontable repo about when they might support commonjs requires, but then I ran into your ticket. From your experience, is it reasonable to expect their maintainers to act on that ticket? It seems they have a lot of open pull requests.

If it's between transpiling only one file and just including another <script> tag, I think the latter is less crufty (and we're already using browserify-shim).

Member

dereklieu commented Jan 5, 2016

Thanks for flagging mobile support. I'm a bit conflicted here between maintaining mobile support for the default Safari that comes on lots of iOS devices. Not sure from their docs what Android support looks like.

Thinking about user expectations, I would see this as an extension of core functionality, so if we left that functionality intact while providing simple fallback for unsupported devices, I think I'd be ok with that.

I was hoping to find some info on the handsontable repo about when they might support commonjs requires, but then I ran into your ticket. From your experience, is it reasonable to expect their maintainers to act on that ticket? It seems they have a lot of open pull requests.

If it's between transpiling only one file and just including another <script> tag, I think the latter is less crufty (and we're already using browserify-shim).

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 5, 2016

Contributor

@dereklieu I noticed all the PRs today too..it doesn't look like they're very active in getting back to folks. It looks like they have a big release coming out on January 11th though: Handsontable pro (per the top bar of their website). Not sure what that will mean for the free version, hopefully good things. Honestly though the main reason I've been using it is that it's so feature rich, only does the one thing (UI) and does it really well, and it appears to have been recently rewritten considering it's in ES2015.

EDIT: Also, when you get a chance, let me know what you think about the styles. (1) should I put my few custom css in any particular place in styles.css? (2) are CSS dependencies concatenated anywhere or is my <link> tag in index.html okay?

EDIT2: Added tests and now realizing that I can't test anything involving Handsontable from node because we're pulling that in via CDN in index.html! (as a result, tests fail, but once the dependency is resolved they should pass and suffice)

EDIT3: I've got it bundling handsontable without parsing it, so I think we're in business. No need for the CDN. Which makes my question above regarding the CSS more relevant.

Contributor

timwis commented Jan 5, 2016

@dereklieu I noticed all the PRs today too..it doesn't look like they're very active in getting back to folks. It looks like they have a big release coming out on January 11th though: Handsontable pro (per the top bar of their website). Not sure what that will mean for the free version, hopefully good things. Honestly though the main reason I've been using it is that it's so feature rich, only does the one thing (UI) and does it really well, and it appears to have been recently rewritten considering it's in ES2015.

EDIT: Also, when you get a chance, let me know what you think about the styles. (1) should I put my few custom css in any particular place in styles.css? (2) are CSS dependencies concatenated anywhere or is my <link> tag in index.html okay?

EDIT2: Added tests and now realizing that I can't test anything involving Handsontable from node because we're pulling that in via CDN in index.html! (as a result, tests fail, but once the dependency is resolved they should pass and suffice)

EDIT3: I've got it bundling handsontable without parsing it, so I think we're in business. No need for the CDN. Which makes my question above regarding the CSS more relevant.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 6, 2016

Contributor

Okay guys, just got unit tests working. As I mentioned in the edits above, doing so involved figuring out how to bundle in Handsontable.js (instead of via <script> tag/CDN). Let me know your thoughts on the questions above when you get a chance.

Update: Added ability to toggle back to the markdown editor, which persists between sessions via cookie

Contributor

timwis commented Jan 6, 2016

Okay guys, just got unit tests working. As I mentioned in the edits above, doing so involved figuring out how to bundle in Handsontable.js (instead of via <script> tag/CDN). Let me know your thoughts on the questions above when you get a chance.

Update: Added ability to toggle back to the markdown editor, which persists between sessions via cookie

@dereklieu

This comment has been minimized.

Show comment
Hide comment
@dereklieu

dereklieu Jan 13, 2016

Member

Hey @timwis sorry for the delay, I'm hoping to have some more time to look at this over the coming days. As far as the CSS goes, you're right in that the repo contains only a parsed file. I'm not sure where the development assets are, but I think it would make sense to include a CSS task in the gulpfile to pull in 3rd-party styles, something akin to a vendor.css.

Can you tell me more about the auto-height issue you mention above?

[edit] to say thanks for figuring out the include and tests - this is coming along really nicely.

Member

dereklieu commented Jan 13, 2016

Hey @timwis sorry for the delay, I'm hoping to have some more time to look at this over the coming days. As far as the CSS goes, you're right in that the repo contains only a parsed file. I'm not sure where the development assets are, but I think it would make sense to include a CSS task in the gulpfile to pull in 3rd-party styles, something akin to a vendor.css.

Can you tell me more about the auto-height issue you mention above?

[edit] to say thanks for figuring out the include and tests - this is coming along really nicely.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 14, 2016

Contributor

Hi @dereklieu, thanks for the reply. Happy to add a gulp task for vendor.css. Is there any other vendor CSS used in the project that's currently in styles.css that should be pulled out while I'm at it?

Regarding the automatic height issue, basically handsontable needs a height style to initialize in this particular way (with horizontal scrolling enabled, which is a key feature for CSV files with a lot of columns). It's currently set to 600px I believe, which looks fine on...well, my laptop. If there are more rows than fit, the handsontable <div> will also scroll vertically, so I think ideally we'd compute the available vertical screen real estate and set the height to that, unless there's a way to do it with position: absolute.

Contributor

timwis commented Jan 14, 2016

Hi @dereklieu, thanks for the reply. Happy to add a gulp task for vendor.css. Is there any other vendor CSS used in the project that's currently in styles.css that should be pulled out while I'm at it?

Regarding the automatic height issue, basically handsontable needs a height style to initialize in this particular way (with horizontal scrolling enabled, which is a key feature for CSV files with a lot of columns). It's currently set to 600px I believe, which looks fine on...well, my laptop. If there are more rows than fit, the handsontable <div> will also scroll vertically, so I think ideally we'd compute the available vertical screen real estate and set the height to that, unless there's a way to do it with position: absolute.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 22, 2016

Contributor

Hey guys, the editor now sets its height based on the available vertical space. There's technically a little extra height but it's still based on the window size and should feel natural from a usability perspective. Any thoughts on merging this?

Contributor

timwis commented Jan 22, 2016

Hey guys, the editor now sets its height based on the available vertical space. There's technically a little extra height but it's still based on the window size and should feel natural from a usability perspective. Any thoughts on merging this?

@dereklieu

This comment has been minimized.

Show comment
Hide comment
@dereklieu

dereklieu Jan 22, 2016

Member

@timwis 👍 sorry for my lateness here. I still haven't had a lot of time to really sit down and review this in full yet, but I'm snowed in all weekend so expect to have time. You'll hear back from me soon.

Member

dereklieu commented Jan 22, 2016

@timwis 👍 sorry for my lateness here. I still haven't had a lot of time to really sit down and review this in full yet, but I'm snowed in all weekend so expect to have time. You'll hear back from me soon.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 23, 2016

Contributor

woohoo! no worries, looking forward to it

Contributor

timwis commented Jan 23, 2016

woohoo! no worries, looking forward to it

@dereklieu

This comment has been minimized.

Show comment
Hide comment
@dereklieu

dereklieu Jan 24, 2016

Member

@timwis code looks great. Thanks for including tests.

One concern I have is that someone might edit a csv with jekyll metadata editing enabled for that directory or globally. Even if you didn't touch the metadata editor, it would still insert default values on save and break that csv for probably every other spreadsheet program.

One option might be to disable the metadata editor if file.useCSVEditor === true. This feels safe to me, as I don't see why anyone would need jekyll metadata for csv files.

From my perspective, that's the only thing we'd need to merge this.

Member

dereklieu commented Jan 24, 2016

@timwis code looks great. Thanks for including tests.

One concern I have is that someone might edit a csv with jekyll metadata editing enabled for that directory or globally. Even if you didn't touch the metadata editor, it would still insert default values on save and break that csv for probably every other spreadsheet program.

One option might be to disable the metadata editor if file.useCSVEditor === true. This feels safe to me, as I don't see why anyone would need jekyll metadata for csv files.

From my perspective, that's the only thing we'd need to merge this.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 24, 2016

Contributor

@dereklieu Good catch. I'm on it! Don't go anywhere :P

Contributor

timwis commented Jan 24, 2016

@dereklieu Good catch. I'm on it! Don't go anywhere :P

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 24, 2016

Contributor

Having a tough time replicating the issue. I'll keep at it.

Contributor

timwis commented Jan 24, 2016

Having a tough time replicating the issue. I'll keep at it.

@dereklieu

This comment has been minimized.

Show comment
Hide comment
@dereklieu

dereklieu Jan 24, 2016

Member

@timwis if you head to http://localhost:3000/#dereklieu/starter/edit/prose-testing/csv/iowa.csv (while serving from localhost:3000), you can see it. Just make a change to the file and try saving.

Member

dereklieu commented Jan 24, 2016

@timwis if you head to http://localhost:3000/#dereklieu/starter/edit/prose-testing/csv/iowa.csv (while serving from localhost:3000), you can see it. Just make a change to the file and try saving.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 24, 2016

Contributor

Thanks @dereklieu - I was able to reproduce it. I agree with the approach you suggested and have implemented it on this PR.

Contributor

timwis commented Jan 24, 2016

Thanks @dereklieu - I was able to reproduce it. I agree with the approach you suggested and have implemented it on this PR.

dereklieu added a commit that referenced this pull request Jan 25, 2016

@dereklieu dereklieu merged commit 376253a into prose:master Jan 25, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dereklieu

This comment has been minimized.

Show comment
Hide comment
@dereklieu

dereklieu Jan 25, 2016

Member

Confirmed, thanks @timwis!

Member

dereklieu commented Jan 25, 2016

Confirmed, thanks @timwis!

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 25, 2016

Contributor

Woohoo! Is there a different process for pushing it live?

Contributor

timwis commented Jan 25, 2016

Woohoo! Is there a different process for pushing it live?

@dereklieu

This comment has been minimized.

Show comment
Hide comment
@dereklieu

dereklieu Jan 25, 2016

Member

Yes, deploying to gh-pages. Have a few things to finish here and will get to that early this afternoon.

Member

dereklieu commented Jan 25, 2016

Yes, deploying to gh-pages. Have a few things to finish here and will get to that early this afternoon.

@dereklieu

This comment has been minimized.

Show comment
Hide comment
@dereklieu

dereklieu Jan 25, 2016

Member

@timwis deployed.

Member

dereklieu commented Jan 25, 2016

@timwis deployed.

@timwis

This comment has been minimized.

Show comment
Hide comment
@timwis

timwis Jan 25, 2016

Contributor

That's great news. Thanks @dereklieu!

Contributor

timwis commented Jan 25, 2016

That's great news. Thanks @dereklieu!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment