Skip to content

Add 'run' functions - #8

Closed
cdevienne wants to merge 7 commits into
miniBill:mainfrom
orus-io:main
Closed

cdevienne wants to merge 7 commits into
miniBill:mainfrom
orus-io:main

Conversation

@cdevienne

Copy link
Copy Markdown

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
Copy Markdown
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

cdevienne commented May 19, 2021 •

Copy link
Copy Markdown
Author

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

miniBill commented Jun 2, 2021 •

Copy link
Copy Markdown
Owner

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

miniBill commented Jun 2, 2021

Copy link
Copy Markdown
Owner

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
Copy Markdown
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
Copy Markdown
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

miniBill commented Jun 7, 2021

Copy link
Copy Markdown
Owner

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

miniBill commented Jun 7, 2021

Copy link
Copy Markdown
Owner

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
Copy Markdown
Author

wouldn't this be covered by embedAttr?

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

@cdevienne

Copy link
Copy Markdown
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

cdevienne commented Jun 14, 2021 •

Copy link
Copy Markdown
Author

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
Copy Markdown
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

miniBill commented Jun 25, 2021 •

Copy link
Copy Markdown
Owner

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

@miniBill

Copy link
Copy Markdown
Owner

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

@miniBill miniBill closed this Jun 25, 2021
@miniBill

Copy link
Copy Markdown
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