reflex-frp / reflex-dom-semui

A reflex-dom API for Semantic UI components
https://reflex-frp.org/
BSD 3-Clause "New" or "Revised" License
22 stars 10 forks source link

Do not use element ids #20

Closed mulderr closed 7 years ago

mulderr commented 7 years ago

Currently for dropdown it's required to specify an element id. It's also suggested that maybe these ids should be generated randomly in the future.

Specifically I'm looking at this.

I am wondering though. Can we just: 1) get the raw element from reflex 2) wrap it in jquery 3) use api functions on that

Instead of $('#elemid').dropdown() use $(rawelem).dropdown(). Not that it matters but it would probably be faster too.

Sort of like this.

Is this a good idea? Would you accept a PR for this?

mightybyte commented 7 years ago

Yes, I'm definitely interested in avoiding the need to pass IDs. I'm not sure of the best way to go about it, but if you come up with a way, odds are good that I'll want to merge it.

mulderr commented 7 years ago

Oh bummer. dropdown does not expose the element. I'll try to get around that somehow.


So good news and bad news. Good news is I tested it on one function and it does seem to work. Bad news is I wrapped everything in a div to have something to refer to. It feels like a big hack and it probably does not make sense to pursue this unless we:

  1. make reflex-dom expose the element created by dropdown so we can use it directly or
  2. duplicate dropdown code in this package with minor changes

I don't like 2. I am not sure how much sense 1. makes for reflex-dom itself. It would be pretty unusual I guess to start exposing this kind of stuff. Thoughts?

mightybyte commented 7 years ago

@ryantrinkle Do you have any thoughts on this?

ryantrinkle commented 7 years ago

Take a look at selectElement in DomBuilder. It's much more flexible.

On Aug 17, 2017 15:54, "Doug Beardsley" notifications@github.com wrote:

@ryantrinkle https://github.com/ryantrinkle Do you have any thoughts on this?

— You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub https://github.com/reflex-frp/reflex-dom-semui/issues/20#issuecomment-323176782, or mute the thread https://github.com/notifications/unsubscribe-auth/ABGlYIsf2JrOMAvBtJNAOl9HnmycaBZkks5sZJpkgaJpZM4O5Sxb .

mulderr commented 7 years ago

I initially rejected selectElement because dropdown is already using it internally, so necessarily there would be a lot of duplication between what we do here and dropdown.

We could fully reuse dropdown with this change to reflex-dom:

dropdown :: forall k t m. (DomBuilder t m, MonadFix m, MonadHold t m, PostBuild t m, Ord k)
  => k -> Dynamic t (Map k Text) -> DropdownConfig t k -> m (Dropdown t k)
dropdown k0 options config = snd <$> dropdown' k0 options config

dropdown' :: forall k t m. (DomBuilder t m, MonadFix m, MonadHold t m, PostBuild t m, Ord k)
  => k -> Dynamic t (Map k Text) -> DropdownConfig t k -> m (SelectElement EventResult (DomBuilderSpace m) t, Dropdown t k)
dropdown' k0 options (DropdownConfig setK attrs) = ...

Also it does not make much sense to go very low level with semantic-ui when using it's javascript API. For example when you activate dropdown on a select element it will hide it using display: none and insert it's own stuff into the DOM to mimic the behavior. I'm pretty convinced that mightybyte's approach of using dyn over dropdown with constant options is actually spot on.

Edit: wait a sec, there's one more thing i'd like to try. In recent versions they added ability to initialize a dropdown with JS only. Would that mean we don't need dropdown at all?

mulderr commented 7 years ago

Eh, I see there's a whole new implementation below that does not use ids. It's also the one used in examples.

The JS only feature is not that exciting since the new implementation already skips the select and builds the DOM semantic UI would insert anyway. And that's a more general approach too.

Maybe the old implementation should be marked deprecated? Are there still some use cases where the new implementation is not sufficient?

I think I understand better what's going on now. Sorry for all the noise.