leptos-rs / leptos

Build fast web applications with Rust.
https://leptos.dev
MIT License
16.79k stars 672 forks source link

Allow for custom attributes and/or node_ref in <A> / <Form> #851

Closed St4NNi closed 1 year ago

St4NNi commented 1 year ago

Hi, first of all thanks for this great framework, i really appreciate it so far !

I am currently developing a web-app that uses leptos in combination with bootstrap. Bootstrap uses many custom attributes like data-bs- and similar and currently I am unable to add these attributes to any "progressively enhanced" components, like <A>, <Form>, <ActionForm> etc. I am working around this by manually creating a <form> and handling all submission / validation logic myself.

Am I missing something ? Is there a way to get to the underlying <form> element from an <Form> or <ActionForm> ? If not this would be a nice addition. Additionally it would also be nice to be able to get a node_ref to the underlying element because especially forms have some convenient methods like web_sys::HtmlFormElement::check_validity()

Thanks

gbj commented 1 year ago

This is a totally reasonable request and something we should be able to do... I certainly don't want you to have to duplicate these components yourself!

I recently added an AdditionalAttributes type for the <Html/> and <Body/> components and we should be able to add something similar to these. node_ref is even easier.

I'll put together a PR in the next couple of days.

Using node_ref with HtmlFormElement::check_validity() is a great idea, by the way.

gbj commented 1 year ago

So I just added this for <Form/> et al in PR #853 but then realized you'd asked about` as well. Do you need the same things there?

Note that the <A/> component only does two things the <a> element does not do:

  1. Correctly resolving relative nested routes
  2. Setting the aria-current attribute if this link is a link to the page you’re on

Plain <a> elements do activate client-side routing, so it's possible that your need for additional attributes or node refs there could be met with <a>. I ask because adding the possibility of additional attributes actually creates a small de-optimization in terms of server-side rendering, so it's better if we can avoid it if possible.

St4NNi commented 1 year ago

Thanks again for the great work and the speedy response. Indeed the benefits for <A> would only be minor compared to <Form> and friends. So I would be fine with it, using plane <a>'s is not a big deal.

gbj commented 1 year ago

Okay cool! I think we're good then, although let me know if anything's broken here of course.

For anyone reading: if you do need to mimic the functionality of <A/> using an <a> because you need additional attributes, you can add the two pieces of functionality yourself fairly easily.

  1. For resolving nested relative routes: use the use_resolved_path function to resolve href relative to the current nested route
  2. For the is_active check, see here https://github.com/leptos-rs/leptos/blob/e20c77710d30bc89da2c759c84f4f43e64701fb3/router/src/components/link.rs#L90-L107
jeanhdex commented 1 year ago

Hey!

I think I'm facing the same issue as described in this first post and I was wondering what is the best way to handle this situation.

Currently using bulma.io as a css framework, I'd like to add classes to a active <a> tag, especially the following one : is-active.

From what I understand, is it not currently possible. Since I'm just starting with leptos, I tried to do a little editing in the router crate to see what I was able to do (see https://github.com/leptos-rs/leptos/commit/7d392a38938e36f733ae57328c477ae80d7b3b20). But actually I'm not very happy with what I did (anyway, forking a crate for a specific need is never a good idea).

I understand the fact that adding this kind of functionality can break some optimizations but also, this kind of need seems very common to me.

From where I am, I see two possibilities:

  1. Rewriting a custom <Link> component for my special need, copying the logic in the original one ;
  2. Dynamically setting classes in the component holding my router, which also sounds under-optimized...

Any thoughts? (@gbj )

gbj commented 1 year ago

@jeanhdex It's possible to style the active link using CSS with the [aria-current=page] selector. Does your CSS framework not allow styling using selectors in this way?

I guess it would be possible to add an active_class prop to the <A/> component. Is that what you're looking for?

jeanhdex commented 1 year ago

Thank you for your fast reply!

Yes I totally agree, this selector might be a solution in general. However for this particular case I thing it would be better to allow user modify the classes on certain conditions. Rewriting (small) fragments of the CSS framework to make it fit in the front-end framework doesn't feel right. Beyond what "feels right" it means risking side effects by changing the CSS logic (not even my code) and imposing this logic to every user of a framework like bulma (I guess other frameworks may have the same logic).

The active_class option is what I went for at the beginning. It would be a great solution. Yet, I didn't find any elegant way of doing it so far. In the tentative fix I did, I had to replace class: Option<AttributeValue> by class: Option<String> to be able to merge properties : usual classes + active classes. (I guess it could be possible with AttributeValue but the code was illegible as there were so many match and indirections).

I also thought of merging them in the <Link> properties but I don't think it is possible. I didn't like yew but there was a pretty convenient classes marco you could use like this:

classes={ classes!(
    "navbar-item",
    if current_page == NavbarCurrentPage::Home { Some("is-active is-tab" ) } else { None }
)}
gbj commented 1 year ago

@jeanhdex Try out the branch in this PR and see if it achieves what you're looking for!

jeanhdex commented 1 year ago

@gbj This works perfectly well! (I didn't test the server side rendering though)

Any reason for using the class method over the classes one? :)

Thank you very much

gbj commented 1 year ago

Any reason for using the class method over the classes one? :)

See this is why we pay you the big bucks.

classes itself only takes a static list of classes, but I've updated this to split a class list with multiple class names so that each class is applied correctly.