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

Parameterize the type of the draggable element's key. #34

Merged
merged 2 commits into from
Feb 12, 2017

Conversation

crazymykl
Copy link
Contributor

@crazymykl crazymykl commented Feb 9, 2017

I am a big fan of this library, and am currently leading a team to rebuild a legacy flash application in Elm. This took care of much of the heavy lifting.

However, I do not like that I have to toString all over the place to "tag" my draggable elements, and then write ugly string-munging functions to get back the original record inside update.

I think other users might also benefit from this increased flexibility.

P. S. As written now, this in 3.0.0; it could potentially be rewritten in such a way as to be 2.1.0, but that seems messier to support two APIs.

P. P. S. I didn't proofread my docs changes super thoroughly yet.

@crazymykl
Copy link
Contributor Author

crazymykl commented Feb 9, 2017

Pretty sure the test failure is a Travis cache making it hit elm-lang/elm-make#133. I can't retry it with a clean cache, but Travis passes on my fork.

@zaboco
Copy link
Owner

zaboco commented Feb 9, 2017

Hi, I'm glad that you find this library useful.

I think other users might also benefit from this increased flexibility.

Yes, it is a common-sense change, I should have thought about it in the first place (I guess I was influenced by Html.Keyed).

P. S. As written now, this in 3.0.0; it could potentially be rewritten in such a way as to be 2.1.0, but that seems messier to support two APIs.

How urgent is this? I plan to make some breaking changes to (hopefully) simplify the API and maybe even solve this issue. I haven't worked on it, it's just an idea so I don't know if it is viable. I want to make a branch to test this in the next week.

Pretty sure the test failure is a Travis cache ...

Yes, I've rerun the build with empty cache and it works.

@crazymykl
Copy link
Contributor Author

I can always publish a fork, but I'd prefer not to. Having another package published forever to soothe a temporary wound is not my favorite idea. (I could also vendor it, but that's also not great...)

I would prefer a 3.0.0 today, and a 4.0.0 next week (or whenever you get to it, life can get in the way of OSS), but I can mange if you'd rather wait and deliver more value in one swell foop.

P. S. Exposing the DOM event object in the Events would be a big help.

@zaboco
Copy link
Owner

zaboco commented Feb 9, 2017

Yes, a fork is not an ideal option.

I'll publish it as soon as possible then, indeed, I cannot predict when I would finish the new API, so no point in blocking.

P. S. Exposing the DOM event object in the Events would be a big help.

There is another PR #33 which adds a customMouseTrigger which gets a JSON decoder for the mouse events. Is this what you had in mind?

@crazymykl
Copy link
Contributor Author

crazymykl commented Feb 9, 2017 via email

@zaboco zaboco merged commit 9a1e188 into zaboco:master Feb 12, 2017
@zaboco
Copy link
Owner

zaboco commented Feb 12, 2017

published as 3.0.0

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.

2 participants