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

FlowContent and PhrasingContent separation is useless #23

Closed
kosiakk opened this issue Sep 20, 2016 · 7 comments
Closed

FlowContent and PhrasingContent separation is useless #23

kosiakk opened this issue Sep 20, 2016 · 7 comments

Comments

@kosiakk
Copy link
Contributor

kosiakk commented Sep 20, 2016

Apparently those contents come from XSD, but in the end of the day, they are quite useless and very confusing in kotlinx.html.

It does not help with static type check

if I write something invalid according to XSD, like body > span > div

body { // BODY creates FlowContent
    span { // SPAN creates PhrasingContent

        div { // DIV can only be created in _FlowContent_, not in SPAN

            // DIV is still allowed, because it is actually created in BODY, 
            // not in innermost parent SPAN
        }
    }
}

it is still silently accepted by the compiler and even leads to obscure IllegalStateException("You can't change tag attribute because it was already passed to the downstream") if I try to change attributes.

To me this seems like a general problem of 'fall-through' builder design.

It breaks the abstraction

Both functions, FlowContent.i and PhrasingContent.i are identical: they produce the same I object.
But there's no common abstraction between those two functions. Which leads to the third problem.

It prevents reusable code

I chose to use Tag as a basis for my DSL, which seemed like a good base class. But in the end I had to write the ugliest code in a decade of my career:

fun Tag.mdl_icon(icon: String, text: String? = null) {
    if (this is FlowContent) {

        i("material-icons") {
            +icon
        }
        if (text != null) {
            span(classes = "visuallyhidden") {
                +text
            }
        }

    } else if (this is PhrasingContent) {

        i("material-icons") {
            +icon
        }
        if (text != null) {
            span(classes = "visuallyhidden") {
                +text
            }
        }
    }
}

Both if and else part are identical, but functions i and span are different.
Condition like if (this is FlowContent || this is PhrasingContent) cannot work, of course.

@kosiakk
Copy link
Contributor Author

kosiakk commented Sep 20, 2016

Very simple suggestion is to declare all tag-functions for the Tag class and discard *Content.
We'll have exactly one SPAN class and exactly one Tag.span function.

Pros:

  • less code to generate
  • simpler implementation, much easier to lean
  • solves all the problems above
  • minimal library, which does one thing (helps to write html from Kotlin)

Cons:

  • hierarchy check still won't work

@olegcherr
Copy link

Just faced the same problem.
Wanted to create an universal helper that would be working both with divs and spans.

@cy6erGn0m
Copy link
Contributor

It is going to be fixed with upcoming release (will be aligned to kotlin 1.1 release)

@cy6erGn0m
Copy link
Contributor

You can use FlowContent or HtmlBlockTag for that purpose. Just need to be documented

@olegcherr
Copy link

@cy6erGn0m Just tested it. And yes, version 0.6+ works very well. Seems like you've completely refactored the inheritance architecture of the library. Thank you!

@cy6erGn0m
Copy link
Contributor

@kosiakk can we close it? Are your use-cases satisfied now?

@kosiakk
Copy link
Contributor Author

kosiakk commented Apr 6, 2017

Yes, FlowContent works for me now, thank you!

@kosiakk kosiakk closed this as completed Apr 6, 2017
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

No branches or pull requests

3 participants