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

WIP: Math ml tag defs #105

Merged
merged 31 commits into from
Dec 28, 2024
Merged

WIP: Math ml tag defs #105

merged 31 commits into from
Dec 28, 2024

Conversation

Quafadas
Copy link

I will not pretend to have written any of this myself - thank you Mr AI.

In the light of the above statement, I'm not clear whether I have a better or worse chance of this actually working.

So let's poke the CI and see what happens.

@Quafadas Quafadas requested a review from raquo as a code owner September 20, 2024 12:11
@davidon-top
Copy link

davidon-top commented Sep 20, 2024

as far as i know org.scalajs.dom doesn't have support for mathml so your scalaJsElementTypeAlias assignments wont work after codegen, i can be mistaken tho i'm not certain

@Quafadas
Copy link
Author

You beat the CI to it!

https://github.com/Quafadas/scala-dom-types/actions/runs/10959385719/job/30431627724

I'm actually not sure whether it needs to be supported by org.scalajs.dom explicitly though.

The browser seems to render it quite happily, so I'm wondering if I just go for some base class and pray?

@Quafadas
Copy link
Author

I guess this is probably going to impact the attributes it can have though? Does that mean I need to make PR to scala-js-dom?

@davidon-top
Copy link

by default sdt generates definitions like these lazy val htmlRootTag: HtmlTag[dom.HTMLHtmlElement] = htmlTag("html") and u can see inside the HtmlTag generic the dom.whatever that is what scalaJsElementTypeAlias describes AFAIK, its probably possible to add it without scala-js-dom having mathml tags but it will need to use a different system/design to make the mathml elements

@davidon-top
Copy link

davidon-top commented Sep 20, 2024

I guess this is probably going to impact the attributes it can have though? Does that mean I need to make PR to scala-js-dom?

i don't think it would impact the attributes since they are basically just a name and codec it would impact tags tho since they accept the generic type from org.scalajs.dom

@Quafadas
Copy link
Author

So I've pattern matched my way through the CI, which is green on my fork. Hopefully it's a starting point for a discussion.

@Quafadas
Copy link
Author

I assume, that in its curren t form, it will exhibit the same set of problems as the original bootlegged HTML. I'm not clear how to work around that though.

Copy link
Owner

@raquo raquo left a comment

Choose a reason for hiding this comment

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

Thanks, that's a good start!

After looking into this a bit, I think overall, we need to treat MathML tags and attributes similarly to how we treat SVG – so they will get their own namespace in Laminar.

Indeed, as you said, for this to be meaningful, someone needs to contribute the MathMLElement interface to scalajs-dom. I think we can do without that if we just use the dom.Element supertype for MathML elements, but that's only suitable for a PoC, it would make the types too loose in Laminar.

I think if we're adding MathML, we should add all of the MathML tags and attributes right away, especially if AI is helping with the chore. Also:

  • The tags and attributes should be sorted alphabetically by their JS / DOM name in the listings
  • The <math> tag itself should be moved to MathMlTags – it's actually a MathMlElement, not an HTMLElement (similar to the root tag)

I made an issue in Laminar to track all the related tasks: raquo/Laminar#172

@raquo raquo mentioned this pull request Sep 24, 2024
10 tasks
@Quafadas
Copy link
Author

@raquo This passes on my fork, I'm not in a position to judge, if the tests are meaningful or not, however :-(

Copy link
Owner

@raquo raquo left a comment

Choose a reason for hiding this comment

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

Thanks, good start! 🚀 Please see comments.

.vscode/settings.json Outdated Show resolved Hide resolved
build.sbt Outdated Show resolved Hide resolved
js/src/test/scala/com/thirdparty/tags/Tag.scala Outdated Show resolved Hide resolved
js/src/test/scala/com/thirdparty/tags/Tag.scala Outdated Show resolved Hide resolved
jvm/src/test/scala/com/raquo/domtypes/GeneratorSpec.scala Outdated Show resolved Hide resolved
@Quafadas Quafadas mentioned this pull request Dec 15, 2024
Copy link
Owner

@raquo raquo left a comment

Choose a reason for hiding this comment

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

Sure, we can merge this into a new mathml branch (pending a couple new comments).

We can merge this into master once the feature is complete and scala-js-dom makes a release.

@Quafadas
Copy link
Author

Is there any chance I'm hitting this?

actions/runner-images#10767

@Quafadas
Copy link
Author

I can't reproduce this failure locally :-(.

@raquo raquo changed the base branch from master to mathml December 28, 2024 23:05
@raquo raquo merged commit da57770 into raquo:mathml Dec 28, 2024
1 check passed
@raquo
Copy link
Owner

raquo commented Dec 28, 2024

I've merged this into the new mathml branch – as we've discussed, we'll keep MathML work in that branch until it's ready to release.

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.

3 participants