feat!: more form attributes#352
Conversation
Codecov Report❌ Patch coverage is
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes 🚀 New features to boost your workflow:
|
4824f19 to
e6b2f3d
Compare
c55425b to
c7ba160
Compare
| /// let rendered = data_list.render(); | ||
| /// ``` | ||
| #[must_use] | ||
| pub fn data_list<I, S>(list: I, id: &str) -> Self |
There was a problem hiding this comment.
I'm not sure if we should force providing id in the constructor, maybe getting rid of it and expecting users to do .attr("id", id") is enough?
There was a problem hiding this comment.
mm, The thing about the datalist element is that the id attr is tightly coupled with the list attribute in the input element (i.e the list attr in the input should match the id attr in the datalist) so if the user forgets to call .attr("id", id) it wont work. I had that as a parameter because it's required for correctness
| /// | ||
| /// [`list`]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#list | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct List(Vec<String>); |
There was a problem hiding this comment.
The linked docs about list attribute for <input> claim that:
The value given to the list attribute should be the id of a element located in the same document.
I understand that your approach was of simplicity, but I think we should first and foremost support the spec and then add conveniences for the users.
I'd say that new should take the id of datalist by default. Or even better, take a datalist struct, but now it's generic HtmlTag, which doesn't make it that much better.
Then, we can provide a convenience constructor, let's say from_list, that has the existing functionality.
There was a problem hiding this comment.
Not sure if I fully understand your suggestion. The List type is used to add data-list options to the input element, so the way i thought of it was, you have an input field that has an id and if you want to specify a data-list for that input type, you can pass in a list of option values as the list attribute value then we implicitly create the data list element with the values from the list as option.
#[derive(Debug, Form)]
struct ExampleForm {
#[form(
opts(
list=List::new(["Title A", "Title B", "Title C"])
)
)]
title,
...
}Which should render into:
<div>
<input type="text" id="title" list="__title_datalist">
<datalist id="__test_datalist">
<option value="Title A"/>
<option value="Title B"/>
<option value="Titile C"/>
</datalist>
</div>The Id lives on the input element and not on the list because the list attribute (which later gets expanded into the datalist element) cant operate independently unless coupled with the input element. Also, keeping the API this way makes it consistent with the signature of other attribute types
There was a problem hiding this comment.
I think it's fine to keep it this way. Typically I expect that you won't need a datalist being used in more than one input element.
m4tx
left a comment
There was a problem hiding this comment.
LGTM, but please fix the minor issues I've mentioned before merging.
|
|
||
| /// Custom options for a [`FileField`]. | ||
| #[derive(Debug, Default, Clone)] | ||
| pub struct FileFieldOptions { |
There was a problem hiding this comment.
Does it make sense to add #[non_exhaustive] on this struct?
| let l = item.as_ref(); | ||
| let mut option = HtmlTag::new("option"); | ||
| option.attr("value", l); | ||
| // option.push_str(l); |
There was a problem hiding this comment.
Please remove the commented out code.
| /// | ||
| /// [`autocomplete`]: https://developer.mozilla.org/en-US/docs/Web/HTML/Reference/Attributes/autocomplete | ||
| #[derive(Debug, Clone)] | ||
| pub enum AutoComplete { |
There was a problem hiding this comment.
We probably want #[non_exhaustive] here, especially since we might want to add explicit support for predefined tokens in the future.
| /// | ||
| /// [`list`]: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#list | ||
| #[derive(Debug, Clone, Default)] | ||
| pub struct List(Vec<String>); |
There was a problem hiding this comment.
I think it's fine to keep it this way. Typically I expect that you won't need a datalist being used in more than one input element.
This PR adds more form attributes to various HTML elements in cot