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

Web: Add Canvas element #1823

Merged
merged 1 commit into from Feb 14, 2022
Merged

Web: Add Canvas element #1823

merged 1 commit into from Feb 14, 2022

Conversation

hfhbd
Copy link
Contributor

@hfhbd hfhbd commented Feb 10, 2022

No description provided.

@Composable
fun Canvas(
attrs: AttrBuilderContext<HTMLCanvasElement>? = null,
content: ContentBuilder<HTMLCanvasElement>? = null
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ContentBuilder is a @Composable lambda. That means it would be possible to write some code like this:

Canvas {
    Div {}
}

I'm not sure that @composable content is applicable for Canvas. I was thinking that canvas' content can be managed only in DisposableEffect or some side effect (at least for now). Or do I miss something?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to MDN you can provide innerHTML content inside a canvas: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/canvas
But mostly for static content as fallback when JS is disabled or the browser does not support canvas. But Compose needs JS and all modern browser (even Internet Explorer :) support canvas. So indeed, this could be removed. Maybe it could take the DisposableEffect as parameter instead:

// current:
Canvas({
  ref {
      Chart(it, jso { })
      onDispose { }
  }
})

// new
Canvas {
  Chart(it, jso { })
  onDispose { }
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW ContentBuilder is required, otherwise you can't use a composable side effect with a reference to the canvas htmlelement. DisposableEffect is not composable, but DOMSideEffect is.

var chartData by remember { mutableStateOf<ChartData>() }
Canvas({}) {
    DomSideEffect(chartData) {
        val chart = Chart(it, config(chartData))
        onDispose {
            chart.destroy()
        }
    }
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DisposableEffect is composable - https://developer.android.com/reference/kotlin/androidx/compose/runtime/package-summary#DisposableEffect(kotlin.Function1)

I think it's worth to allow everyone use as many effects as they want inside Canvas {}, so I agree that content is needed for this.
I still don't want allow calling Div {} or any html element within canvas content. But I don't see a way to prohibit it now.

Thanks! lgtm

Copy link
Contributor Author

@hfhbd hfhbd Feb 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did I miss something?

* [ref] can be used to retrieve a reference to a html element.
* The lambda that `ref` takes in is not Composable. It will be called only once when an element added into a composition.
* Likewise, the lambda passed in `onDispose` will be called only once when an element leaves the composition.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that's right fun ref(...) is a function in AttrsScope and it's not composable https://github.com/JetBrains/compose-jb/blob/7ea30be92404ef4dbdd679cca0b88970b6f3b12e/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/AttrsScope.kt#L82

In your comment above you mentioned that "DisposableEffect is not composable", so I posted the link to docs where it shows that fun DisposableEffect(...) has composable annotation.

Anyway, you're right, one can't use any composable function within ref {} lambda.

@eymar
Copy link
Collaborator

eymar commented Feb 11, 2022

@shabunc What do you think? Does it conflict with your work in progress related to canvas?

@eymar eymar requested a review from Schahen February 11, 2022 09:47
Copy link
Collaborator

@Schahen Schahen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only thing I ask to rollback is imports - apart from that I agree with Philip - we should allow this element to have content

@hfhbd hfhbd requested a review from Schahen February 12, 2022 11:41
@eymar eymar merged commit a528838 into JetBrains:master Feb 14, 2022
@hfhbd hfhbd deleted the hfhbd/canvasElement branch February 14, 2022 11:40
eymar pushed a commit that referenced this pull request Feb 14, 2022
Co-authored-by: hfhbd <hfhbd@users.noreply.github.com>
(cherry picked from commit a528838)
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.

None yet

3 participants