rstudio / sortable

R htmlwidget for Sortable.js
https://rstudio.github.io/sortable/
Other
130 stars 30 forks source link

Add input callback #33

Closed colearendt closed 5 years ago

colearendt commented 5 years ago

Make callback configurable Add example application with configurable callback and rich content Close #31

colearendt commented 5 years ago

Oops. Looks like this will need to be re-added back on top once #28 lands πŸ˜…

colearendt commented 5 years ago

Rebased on top of #28 . You can change the target to go into that PR if you expect it to live a bit longer and want to include these changes.

I haven't rebuilt the docs yet to simplify the merging / rebasing process πŸ€·β€β™‚ (which is why the build is failing)

codecov-io commented 5 years ago

Codecov Report

Merging #33 into master will increase coverage by 0.08%. The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #33      +/-   ##
=======================================
+ Coverage   86.91%   87%   +0.08%     
=======================================
  Files          11    11              
  Lines         298   300       +2     
=======================================
+ Hits          259   261       +2     
  Misses         39    39
Impacted Files Coverage Ξ”
R/js.R 100% <100%> (ΓΈ) :arrow_up:

Continue to review full report at Codecov.

Legend - Click here to learn more Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data Powered by Codecov. Last update 643200e...5d4f7b0. Read the comment docs.

schloerke commented 5 years ago

@colearendt What about looking for an id of the child and if no id exists, it returns the innerText?

colearendt commented 5 years ago

The weird thing is that the id of the child is often meaningless (we define that element). It’s the id of the grandchild that might be interesting.

Sent from my iPhone

On Jul 22, 2019, at 12:39 PM, Barret Schloerke notifications@github.com wrote:

@colearendt What about looking for an id of the child and if no id exists, it returns the text?

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

schloerke commented 5 years ago

So there's two things at play here.

  1. I don't want to allow people to have their own callback. Instead I'd rather look for an id or return the inner text.
  2. Currently we have a default div that contains the whole child. Should remove that if the item being supplied is an htmltag? (vs a character value)
    • This would allow the direct setting of values without an item container div
    • Then we'd only need the child's id value.

@colearendt , Does this sound ok?

colearendt commented 5 years ago

Ahh yes, that makes sense. And yeah, I think that'd be pretty awesome - would that work for sortable.js? I thought the class was important on that outer div to tell sortable.js that this is an "item" for the list. Would you set that class on the user's provided object? What happens w/ htmlTag being longer than a single element? I.e. I wrapped in list() to protect against issues.

I'm definitely fine with parsing for the grandchild's id, by the way. It's just a little weird πŸ˜„

schloerke commented 5 years ago

It should work. Mind if I take this PR over?

colearendt commented 5 years ago

Not at all! Please do πŸ˜„

Sent from my iPhone

On Jul 22, 2019, at 3:58 PM, Barret Schloerke notifications@github.com wrote:

So there's two things at play here.

I don't want to allow people to have their own callback. Instead I'd rather look for an id or return the inner text. Currently we have a default div that contains the whole child. Should remove that if the item being supplied is an htmltag? (vs a character value) This would allow the direct setting of values without an item container div Then we'd only need the child's id value. @colearendt , Does this sound ok?

β€” You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.