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

Add 'run' functions #8

Closed
wants to merge 7 commits into from
Closed

Add 'run' functions #8

wants to merge 7 commits into from

Conversation

cdevienne
Copy link

When wrapping third-party views that takes vanilla element we need to locally render a element with context.

cdevienne added 2 commits May 12, 2021 12:31
When wrapping a third-party view that takes native elm-ui elements, we must be able
to render a with-context element into a native element.
@miniBill
Copy link
Owner

I... don't know 🤔
On the one hand it's pretty obvious that there is a need for escape hatches when interacting with third party code. On the other hand I've explicitly omitted the run function so far.

If exposed I wouldn't call it run I think.

Considering that I'm not planning on doing further edits on the library I'm somewhat convinced that you'd be better off just vendoring it.

On the other hand you're not the first one that has a need for such an hatch (pinging @lucamug), so I'm seriously considering what to do 🤔

Let me think about this for a bit.

@cdevienne
Copy link
Author

cdevienne commented May 19, 2021

Vendoring is what I do in the mean time but the fact that there is a valid use case for those functions that has no other work-around justifies the addition.

elm-html-with-context provides toHtml (and "fromHtml").

How about "toElement", "toElementAttribute" etc ?

@miniBill
Copy link
Owner

miniBill commented Jun 2, 2021

How about this API?

embed :
    ({ unwrap : Element context msg -> Element.Element msg
     , unwrapAttr : Attr context decorative msg -> Element.Attr decorative msg
     }
     -> Element context msg
    )
    -> Element context msg

[and a corresponding embedAttr]

@miniBill
Copy link
Owner

miniBill commented Jun 2, 2021

Try this branch and see if the API is general enough for you: https://github.com/miniBill/elm-ui-with-context/tree/embed-api

@cdevienne
Copy link
Author

Hi @miniBill

The API is nice, and meet my need (I am writing a elm-ui-widgets-with-context) if we complement it a little.

I would need additional unwrappers: unwrapPlaceholder, unwrapLabel, unwrapThumb, unwrapOption (I may have forgotten some)
Also, adding the context itself would make thinks easier and avoid having to nest a 'with' inside a 'embed' every time.
This would give:

embed :
    ({ context : context
     , unwrap : Element context msg -> Element.Element msg
     , unwrapAttr : Attr context decorative msg -> Element.Attr decorative msg
     , unwrapPlaceholder : Placeholder context msg -> Element.Input.Placeholder msg
     , unwrapLabel : Label context msg -> Element.Input.Label msg
     , unwrapThumb : Thumb context -> Element.Input.Thumb
     , unwrapOption : Option context value msg -> Element.Input.Option value msg
     }
     -> Element context msg
    )
    -> Element context msg

@cdevienne
Copy link
Author

I just encoutered a case where the new 'embed' may not be enough.
I want to wrap this function:

singleModal : List { onDismiss : Maybe msg, content : Element.Element msg }
  -> List (Element.Attribute msg)

as

singleModal : List { onDismiss : Maybe msg, content : Element context msg }
  -> List (Attribute context msg)

I don't see a way to do that with "embed". Any idea ?

@miniBill
Copy link
Owner

miniBill commented Jun 7, 2021

I just encoutered a case where the new 'embed' may not be enough.
I want to wrap this function:

singleModal : List { onDismiss : Maybe msg, content : Element.Element msg }
  -> List (Element.Attribute msg)

as

singleModal : List { onDismiss : Maybe msg, content : Element context msg }
  -> List (Attribute context msg)

I don't see a way to do that with "embed". Any idea ?

wouldn't this be covered by embedAttr?

@miniBill
Copy link
Owner

miniBill commented Jun 7, 2021

Hi @miniBill

The API is nice, and meet my need (I am writing a elm-ui-widgets-with-context) if we complement it a little.

I would need additional unwrappers: unwrapPlaceholder, unwrapLabel, unwrapThumb, unwrapOption (I may have forgotten some)
Also, adding the context itself would make thinks easier and avoid having to nest a 'with' inside a 'embed' every time.
This would give:

embed :
    ({ context : context
     , unwrap : Element context msg -> Element.Element msg
     , unwrapAttr : Attr context decorative msg -> Element.Attr decorative msg
     , unwrapPlaceholder : Placeholder context msg -> Element.Input.Placeholder msg
     , unwrapLabel : Label context msg -> Element.Input.Label msg
     , unwrapThumb : Thumb context -> Element.Input.Thumb
     , unwrapOption : Option context value msg -> Element.Input.Option value msg
     }
     -> Element context msg
    )
    -> Element context msg

yeah, all the other unwraps definitely make sense and adding the context makes sense too!

@cdevienne
Copy link
Author

wouldn't this be covered by embedAttr?

Possibly. The list annoys me, I have to try harder.

@cdevienne
Copy link
Author

Another thought: the embed function will probably need to be in a dedicated module to avoid circular dependencies.
I was thinking "Element.WithContext.Embed", and the functions would be : "el", "attr", etc.

And it needs some changes in 'Input', possibly by moving things in Internal, to forge the unwrap functions.

@miniBill Do you intend to work on it ? or should I try and do another PR ?

@cdevienne
Copy link
Author

cdevienne commented Jun 14, 2021

I think wrapping complex apis like elm-ui-widgets might be impossible without rewriting big parts of it and end up in a maintenance nightmare.
So I decided to try and fork the project, only based on elm-ui-with-context. It feels easier, will see.

@cdevienne
Copy link
Author

@miniBill I extended your branch to have all the unwrap function, but I hit another wall.
I want to wrap a view [1] that takes 2 different 'msg' types: "msg", and "Never".

With the embed api you proposed I cannot unwrap the 'Attribute context Never' attributes, unless we add some extra "unwrapNeverAttribute" function.

To conclude, this embed API is perfect for simple cases but is not able to handle all the cases like exposing 'run' functions would.

Let me know what's your opinion on this.

[1] https://package.elm-lang.org/packages/fabhof/elm-ui-datepicker/latest/DatePicker#input

@cdevienne cdevienne mentioned this pull request Jun 16, 2021
@miniBill
Copy link
Owner

miniBill commented Jun 25, 2021

Can't you just mapAttribute never the Attribute context Never before unwrapping?

@miniBill
Copy link
Owner

[I'm going to close this in favor of #10]

@miniBill miniBill closed this Jun 25, 2021
@miniBill
Copy link
Owner

And I do agree that having the unwrapping functions makes sense in the same way that having html and htmlAttribute make sense for elm-ui as an escape hatch

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