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

Allow users to edit datastructures #8

Merged
merged 7 commits into from Jun 25, 2014
Merged

Conversation

travis
Copy link
Contributor

@travis travis commented Jun 23, 2014

Add an "edit" button to collection views. Users can edit and save EDN and the underlying datastructures will be updated.

I had to be careful with the save process because

(om/update! data new-data)

triggers a re-mount of the component, which caused an error when we tried to set state on the newly unmounted component.

Also add support for displaying dates.

This addresses #5 and uses callbacks rather than channels because it felt simpler. Ideally I'd like to find a way to factor the editing component out into its own subcomponent, but it ends up pretty intertwined because of the ways the current state of the editor changes what should display. Thoughts welcome!

Travis Vachon added 5 commits June 23, 2014 14:57
Add an "edit" button to collection views. Users can edit and save EDN
and the underlying datastructures will be updated.

I had to be careful with the save process because

(om/update! data new-data)

triggers a re-mount of the component, which caused an error when we
tried to set state on the newly unmounted component.
@noprompt
Copy link
Owner

Damn! This looks great!

uses callbacks rather than channels because it felt simpler

Thank you! Yes. There's no need to reach for core.async all the time. Beside, callbacks are a great base and if someone wants core.async then it's pretty easy to install.

So I just checked this out and I'm so excited to merge it but I have a couple questions:

  • When clicking the edit button to toggle the editor could we .focus it?
  • I'm half and half on this one but when the editor loses focus could we assume save?
  • Could we get the whole monospace font bit going for the text field?

Overall, you made my day with this PR. 👍

:fontWeight "bold"
:padding "0"
:opacity (if disable? "0.5" "1.0")}}
(if (om/get-state owner :editing?) "save" "edit")))
Copy link
Owner

Choose a reason for hiding this comment

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

Capitalize please ("Save", "Edit").

@sgrove
Copy link

sgrove commented Jun 24, 2014

Few ideas:

  1. Don't show and edit for non-editable data (e.g. #js{}), or at least differentiate
  2. Data-specific inputs - if editing a leaf with a primitive/serializable data-type, then use an input-type that matches. Probably needs to be to be changed to raw input when the user decides to change types, but it'd be great to have an e.g. range control for numbers to be able to scrub in-app, a date-picker, etc.
  3. Right now it looks like the reader only take the first token in the text box, e.g. edit #{"a" "b" "c"} edit => 1 #{"a" "b" "c"} confirm => 1. There's no way to undo by default either, so perhaps it's better to catch it as another error?
  4. In the example, -edit#example.Person{:first-name"Bill" :last-name"Braskey"} opens a text editor, but then won't save because Could not find tag parser for example.Person in ("inst" "uuid" "queue" "js"). Pretty reasonable, but probably shouldn't turn into a text box/be editable. Perhaps there's a way of the user providing extensions to handle custom types/tags?

Overall, what's there in the PR is definitely better than not having it in at all, and it's awesome of you to have done this @travis - thank you!

@travis
Copy link
Contributor Author

travis commented Jun 24, 2014

Glad I could help! Thanks to @dwwoelfel for giving me the task, it was super fun.

@noprompt
Copy link
Owner

@sgrove Awesome notes. Thanks for investigating this PR deeply. If @travis wants to make some more patches or at least fix the recommendations in the code review notes, I don't see why we can't begin merging this now and push out a SNAPSHOT. Sean, if you have time to assist in any way that'd be awesome too. The ball is rolling now so we've got something to work with.

- focus editor on edit, save on blur
- copy tweaks
- predicates to make key detection read better
- code formatting tweaks
@travis
Copy link
Contributor Author

travis commented Jun 24, 2014

yeah, agree - great thoughts, but I probably won't get to them today. I've updated from @noprompt's recommendations and am experimenting a bit with some of the @sgrove's ideas, I'll file enhancements for them with some thoughts.

@noprompt
Copy link
Owner

@travis Awesome. So can I merge this and release a SNAPSHOT or would you prefer I hold off until you've got a bit more? We can always do another PR. No big deal.

@travis
Copy link
Contributor Author

travis commented Jun 25, 2014

yeah, definitely down with merging now and putting up more prs with refinements

@noprompt
Copy link
Owner

@travis Cool. I'll merge this. Thanks again! And send me more patches! 😉

noprompt added a commit that referenced this pull request Jun 25, 2014
Allow users to edit datastructures
@noprompt noprompt merged commit bef3d0d into noprompt:master Jun 25, 2014
@noprompt noprompt mentioned this pull request Jun 25, 2014
@travis
Copy link
Contributor Author

travis commented Jun 25, 2014

arg, apparently I broke the save button somehow. will fix and pr.

@noprompt
Copy link
Owner

No worries. I'll be around. 👍

@travis
Copy link
Contributor Author

travis commented Jun 25, 2014

huh, or maybe not. saw it once, might have been old code? something to keep an eye out for.

@noprompt
Copy link
Owner

Just pushed up the 0.1.4-SNAPSHOT if you wanna verify.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants