PaackEng / elm-ui-dropdown

Dropdown module for Elm UI
https://package.elm-lang.org/packages/PaackEng/elm-ui-dropdown/latest/
MIT License
20 stars 11 forks source link

Suggestion: `update` API improvements #45

Open chrisfls opened 3 years ago

chrisfls commented 3 years ago

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

InternalConfig record type

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

update function

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

And nothing else is needed here.


What do you think?

Let's bring this API to pristine state 💪

PedroHLC commented 3 years ago

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!

chrisfls commented 3 years ago

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 commented 3 years ago

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