elm-ui-dropdown icon indicating copy to clipboard operation
elm-ui-dropdown copied to clipboard

Suggestion: `update` API improvements

Open chrisfls opened this issue 4 years ago • 3 comments

I have buncha feedback suggestions from integrating this library with paack-ui, and also my previous experience integrating Elm with deno's HTTP


Dropdown module

Effect type

  • Remove Loopback
  • Add Select item (or a better name)

InternalConfig record type

  • Remove onSelectMsg
    • Remove every propagation of it

Because the new effect already captures all the work onSelectMsg was doing.

update function

  • Remove Config item msg model parameter

  • Add { onSelect : item -> Cmd msg } (or a better name) as the first parameter

Because keeping the Config here requires the update flow to know too much about the view.

Also, the new { onSelect : item -> Cmd msg } can avoid a Task.perform Task.succeed loopback 🙌

updateWithoutPerform function

  • Remove Config item msg model parameter

And nothing else is needed here.


What do you think?

Let's bring this API to pristine state 💪

chrisfls avatar Jun 16 '21 16:06 chrisfls

Remove Loopback; Add Selected item (or a better name)

I disagree, it's not very clear how one should implement that effect (or their simulation) on their own...

Remove Config item msg model parameter

oh God, please do this!

PedroHLC avatar Jun 16 '21 16:06 PedroHLC

I disagree, it's not very clear how one should implement that effect (or their simulation) on their own...

It doesn't matter "how one should implement that effect", we should not expect to have a canonical implementation for every effect. For this one, the outcome is up to the user.

We just created a lot of effects with canonical implementations elsewhere because it's convenient for their purpose (E2E testing).


oh God, please do this!

We'll have to remove loopback to do that.

chrisfls avatar Jun 16 '21 17:06 chrisfls

~~I've updated the issue to present an alternative that might make you happy @PedroHLC~~

EDIT: nah, forget about it, adding a msg back to Effect would require the onLoopback to be available for updateWithoutPerform and I'm not happy with that

chrisfls avatar Jun 16 '21 20:06 chrisfls