Skip to content

Conversation

@phoebe100
Copy link
Contributor

Fixes #5770

<:actions>
<.link href={~p"<%= schema.route_prefix %>/new"}>
<.button>New <%= schema.human_singular %></.button>
<.link href={~p"<%= schema.route_prefix %>/new"} class="py-2 px-3 text-sm font-semibold rounded-lg bg-zinc-100 hover:bg-zinc-200/80">
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I suggest this style since a very similar style is already used for the button-alike link "Get Started" in app.html.heex

Copy link
Member

Choose a reason for hiding this comment

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

We should avoid using tailwind styles in generators. The home page is fine, because we discard it. Could we use <.button> only instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my projects I have a <.button link={%{patch: ...}} that then has a properly styled <.link inside. Maybe that's the way to go?

Copy link
Contributor Author

@phoebe100 phoebe100 May 21, 2024

Choose a reason for hiding this comment

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

What about plain old link by simply using <.link> without any fancy style?
Buttons would always require javascript and some users might create their projects with --no-live flag.

Or the generators could produce different elements:

  • mix phx.gen.live could use <.button> with phx-click only
  • while mix phx.gen.html could stick to simple <.link> (which could still be styled/replaced further if CoreComponents gets some button-alike link component in the future)

SteffenDE added a commit that referenced this pull request May 21, 2024
Nesting a button inside an anchor tag is not valid HTML. This change
introduces a new `link` attribute in the `button` core component that,
when present, will render the button as an anchor tag instead.

Fixes #5770.
References #5814.
SteffenDE added a commit that referenced this pull request May 21, 2024
Nesting a button inside an anchor tag is not valid HTML. This change
introduces a new `link` attribute in the `button` core component that,
when present, will render the button as an anchor tag instead.

Fixes #5770.
References #5814.
SteffenDE added a commit that referenced this pull request May 21, 2024
Nesting a button inside an anchor tag is not valid HTML. This change
introduces a new `link` attribute in the `button` core component that,
when present, will render the button as an anchor tag instead.

Fixes #5770.
References #5814.
SteffenDE added a commit that referenced this pull request May 21, 2024
Nesting a button inside an anchor tag is not valid HTML. This change
introduces a new `link` attribute in the `button` core component that,
when present, will render the button as an anchor tag instead.

Fixes #5770.
References #5814.
@lifeiscontent
Copy link
Contributor

lifeiscontent commented Oct 23, 2024

👋🏻 new to Elixir & Phoenix, but the way the industry has been solving the (I need button styles for my link) problem is by using a prop called asChild in React. e.g. <Button asChild><a /></Button> where the <Button> would forward its props onto the a, is this something thats possible for Phoenix components? if yes, this might be a good solution to the problem.

SteffenDE added a commit that referenced this pull request Nov 26, 2024
It is not allowed to render a button inside an <a> tag:

https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element

Therefore, we invert the order of the elements and nest a link inside a
button and forward any button clicks to the link.

Fixes #5770.
References #5814.
SteffenDE added a commit that referenced this pull request Nov 26, 2024
* prevent invalid markup in generators

It is not allowed to render a button inside an <a> tag:

https://html.spec.whatwg.org/multipage/text-level-semantics.html#the-a-element

Therefore, we invert the order of the elements and nest a link inside a
button and forward any button clicks to the link.

Fixes #5770.
References #5814.

* update LV in integration tests
@SteffenDE
Copy link
Contributor

Closed via 1f4be18. Thank you!

@SteffenDE SteffenDE closed this Nov 26, 2024
@lifeiscontent
Copy link
Contributor

lifeiscontent commented Dec 3, 2024

@SteffenDE I've come up with an idea I think might be a good fit for phoenix defaults, in my own codebase I've created a link_button which is the exact same thing as button but just uses link under the hood instead. for all the styling I've abstracted the classes out into private helpers, if you think this might be a reasonable approach for phoenix defaults, I can open a PR. as the commit in 1f4be18 is still considered invalid HTML, you cannot nest interactive elements inside one another

@SteffenDE
Copy link
Contributor

@lifeiscontent This sounds similar to what I proposed initially in #5820 (I added a link attribute to the button 89c76df, but another idea was a button_link). We ultimately decided against it because we didn't want to mirror the link component's API.

@lifeiscontent
Copy link
Contributor

@SteffenDE the current solution is still producing invalid HTML though, you can't nest button -> a or a -> button and doing either may cause some bizarre bugs

@SteffenDE
Copy link
Contributor

@lifeiscontent you‘re absolutely right and now I feel stupid 😃
I also somehow missed the part of your comment where you already mentioned the commit. I’m not sure why I thought only nesting the one way wasn’t valid, so back to square one…

@josevalim
Copy link
Member

@SteffenDE should we revert this? Or this is an improvement anyway?

@SteffenDE
Copy link
Contributor

No need to revert, it’s basically the same, but I plan to send a new PR

@lifeiscontent
Copy link
Contributor

lifeiscontent commented Dec 26, 2024

@SteffenDE maybe a little far out of an idea, but what if we prefixed all of the core components with core_* e.g. core_button core_link that way people would know they're getting styled things by the core_components.

this has some interesting growth benefits. e.g. if its <.link /> or <.button /> you know those are system integrations.

and if people want to completely overhaul their design systems, they can naturally graudate to something like app_*

@phoebe100 phoebe100 deleted the 5770_remove_invalid_html_in_templates branch August 20, 2025 15:08
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.

HTML and Live CRUD generators output invalid HTML (buttons nested in links)

4 participants