Prevent invalid markup in generators

[SteffenDE]

Nesting a button inside an anchor tag is not valid HTML. This change inverts the order of the elements and nests a link inside a button instead. We forward any button clicks to the link.

Fixes #5770.
References #5814.

[josevalim]

Good question! it doesn’t feel great if we have to mirror the .link API.

[Gazler]

I prefer allowing:

<button patch/href/navigate={...}>

because it doesn't require the user to learn a new API.

Another option I have used before is a button_link/1 component which mirrors the link API.

[chrismccord]

LiveBeats (which predates core components) also implemented a button patch, so while mirroring link doens't feel great, I've been down this path and I guess that's my preference ¯_(ツ)_/¯
https://github.com/fly-apps/live_beats/blob/master/lib/live_beats_web/components/core_components.ex#L516

[SteffenDE]

So if we mirror the link API, for full link support we'd need to add

• href
• patch
• navigate
• replace
• method
• csrf_token

If you feel like this is the way to go I'll adjust the PR. Definitely doesn't feel nice, especially as users could also write something like

<.button type="submit" replace>

so we'd probably want to have a runtime check that raises if some of these are used without href/patch/navigate?

With all this considered I think I'd prefer either the <.button link={} or a <.button_link like

@doc """
Renders a link, styled like a button.

For available options, see `Phoenix.Component.link/1`.
"""

attr :class, :string, default: ""
attr :rest, :global, include: ~w(href patch navigate replace method csrf_token download hreflang referrerpolicy rel target type)

def button_link(assigns) do
  ~H"""
  <.link
    class={[
      "phx-submit-loading:opacity-75 rounded-lg bg-zinc-900 hover:bg-zinc-700 py-2 px-3",
      "text-sm font-semibold leading-6 text-white active:text-white/80",
      @class
    ]}
    {@rest}
  >
    <%= render_slot(@inner_block) %>
  </.link>
  """
end

which always renders as <a>.

[josevalim]

I am rethinking this discussion. Looking at the attributes:

attr :rest, :global, include: ~w(href patch navigate replace method csrf_token download hreflang referrerpolicy rel target type)

It doesn't feels href/patch/navigate should really not be part of a button? Why are we navigating on button actions? I know some people use it for login/sign up buttons but 1. we don't have to do it by default 2. nor push people towards this direction by having this as a built-in?

[SteffenDE]

I think one reason why people often want to navigate using buttons is to keep state in the URL. One big footgun is deploying a LV app which sooner or later forces clients to reconnect and then lose state if it is not kept in the URL or a form (and more complex use cases). So at least from my experience, very often when clicking a non-submit button something in the URL will (or at least should) change, to have a good user experience.

We could also just use <.button phx-click={JS.navigate()} for the generators, but this also degrades UX because it won't allow thinks like right clicking and copying the URL / opening in a new tab, etc.

So I do think that sooner or later complex applications that have styled buttons will be in the situation where they want a button_link-thingy.

[phoebe100]

I like the idea of button_link the most.
CoreComponents currently just lacks a beautifully styled link and when reading the code, it is far more obvious, that the component is rendered to an html <a> instead of a <button>

[josevalim]

After some thoughts. I would just change the markup to use something else. We should aim towards making CoreComponents smaller, not larger. Many people already think it is quite intimidating. :)

[rhcarvalho]

While Core Components give a great out-of-the-box start, I've felt at times that overriding the default classes is unexpectedly hard. A change I remember doing in a project was making it easier to override all classes for some components, <.link> included.

Reading the present conversation, it seems to me that if it was easier to override the styles of each component a potential button_link would simply be a shortcut to a link with a set of project-specific CSS classes, and so we'd separate the choice of HTML tag from the choice of appearance and avoid duplicating APIs.

At present, when a component take a :class attribute it is additive. How about introducing a standard way to make it replace any defaults?

One way would be to make the default styles available (perhaps as functions), and have :class aways override all styles. To add to existing styles one would call the function for the default styles and add (or remove/edit as any other string) before calling the component.

Reminds me that on occasion I have resorted to using Tailwind's ! prefix to make some important CSS classes to override default core component styles :)

[rhcarvalho]

Reading through other issues I ran into phoenixframework/phoenix_live_view#2590, which shows exactly what I was describing regarding overriding classes.

[leejarvis]

After some thoughts. I would just change the markup to use something else. We should aim towards making CoreComponents smaller, not larger.

I agree with this. The links also don't actually need to look the same as the button generated by CoreComponents.button/1. Just having an inline class on the 4 "link buttons" in the generator seems fine?

[SteffenDE]

@leejarvis we don't want to add Tailwind classes to the templates, see #5814 (comment).

So I'm not sure what best to do. If we just use Phoenix.Component.link/1 it is not styled and doesn't look nice:

But I also don't like having a <.styled_link navigate={~p"/foo"}>...</.styled_link> because we then need to mirror the Phoenix.Component.link/1 API...

Ideally, we'd have a way to specify that a component wraps another one and accepts all of its attributes:

include_attrs, from: {Phoenix.Component, :link}
include_slots, from: {Phoenix.Component, :link}

def styled_link(assigns) do
    ~H"""
    <.link class="default-styles" {assigns} />
    """
end

and still get all compile time validations if we write <.styled_link foo={:bar} />.

[leejarvis]

So I'm not sure what best to do. If we just use Phoenix.Component.link/1 it is not styled and doesn't look nice

I agree that the button style looks a little nicer, but normal links are already used in other places (like show/edit/delete). I think the emphasis on this element being a "normal" link beats having custom styles or over-populating CoreComponents. Just my .2 cents though 🙂

[josevalim]

Another approach we could try to find is to find a style that works both for "Back to Posts" and "Edit posts". Then instead of introducing a new function, we replace <.back by something that is generally more applicable.

[SteffenDE]

Another approach we could try to find is to find a style that works both for "Back to Posts" and "Edit posts". Then instead of introducing a new function, we replace <.back by something that is generally more applicable.

I actually wanted to do this, but then I needed to mess with the margin included in the back component.

For now, I updated the PR to use unstyled links.

[leejarvis]

Another approach we could try to find is to find a style that works both for "Back to Posts" and "Edit posts". Then instead of introducing a new function, we replace <.back by something that is generally more applicable.

We could replace back with the aforementioned button_link and then allow an optional icon attr on that function, then use the same style for back buttons with <button_link navigate={...} icon="hero-arrow-left-solid">Back to Posts</.button_link> and omit icon for the other button links. Might be over-engineering things though.

I support this change as it is though given that it fixes the invalid markup issue.

[SteffenDE]

Another option to prevent invalid HTML would be to just reverse the order of elements:

~H"""
<.button phx-click={JS.dispatch("click", to: "a", inner: true)}>
  <.link navigate={~p"/posts/new"}>New Post</.link>
</.button>
"""

You could also do this without the dispatch, but then clicking on the outer edge of the button would do nothing.

This requires phoenixframework/phoenix_live_view#3396.

[SteffenDE]

@josevalim I inverted the order of elements and use the new element targeting syntax introduced in a recent LV RC. Wdyt?