Replace the input! macro with an Input builder #646

trinity-1686a · 2019-08-21T17:49:45Z

elegaanz pievienoja piebildi

2019-08-21 17:49:45 +00:00

(Pārcelta no github.com)

Pros:

less ambiguities
more type safety
less cloning (we don't clone forms for each input anymore)

Cons:

more verbose (but at the same time it is easier to parse IMO)
less natural syntax

Pros: - less ambiguities - more type safety - less cloning (we don't clone forms for each input anymore) Cons: - more verbose (but at the same time it is easier to parse IMO) - less natural syntax

codecov[bot] pievienoja piebildi

2019-08-21 18:02:20 +00:00

(Pārcelta no github.com)

Codecov Report

Merging #646 into master will decrease coverage by 0.26%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #646      +/-   ##
==========================================
- Coverage   35.06%   34.79%   -0.27%     
==========================================
  Files          68       68              
  Lines        7945     8006      +61     
  Branches     1890     1894       +4     
==========================================
  Hits         2786     2786              
- Misses       4383     4444      +61     
  Partials      776      776

# [Codecov](https://codecov.io/gh/Plume-org/Plume/pull/646?src=pr&el=h1) Report > Merging [#646](https://codecov.io/gh/Plume-org/Plume/pull/646?src=pr&el=desc) into [master](https://codecov.io/gh/Plume-org/Plume/commit/a6c84daa1a242246ac01260eb9bc201dd6e47830?src=pr&el=desc) will **decrease** coverage by `0.26%`. > The diff coverage is `0%`. ```diff @@ Coverage Diff @@ ## master #646 +/- ## ========================================== - Coverage 35.06% 34.79% -0.27% ========================================== Files 68 68 Lines 7945 8006 +61 Branches 1890 1894 +4 ========================================== Hits 2786 2786 - Misses 4383 4444 +61 Partials 776 776 ```

igalic (Pārcelta no github.com) izskatīja 2019-08-21 18:50:06 +00:00

igalic (Pārcelta no github.com) pievienoja piebildi

this looks pretty cool!

trinity-1686a pievienoja piebildi

2019-08-21 19:03:36 +00:00

Īpašnieks

Maybe implementing https://docs.rs/ructe/0.6.2/ructe/templates/trait.ToHtml.html would make more sens than a stand-alone function? Also, clippy is complaining about conventions, to_* usually take by reference

👍 1

elegaanz pievienoja piebildi

2019-08-21 19:40:45 +00:00

(Pārcelta no github.com)

I thought about it, but my function takes a gettex::Catalog, while ToHtml::to_html doesn't. I could have passed it as an argument to Input::new tho.

But I made the commit before reading your review, so it is Input::html now (I can still change it if you prefer).

I thought about it, but my function takes a `gettex::Catalog`, while `ToHtml::to_html` doesn't. I could have passed it as an argument to `Input::new` tho. But I made the commit before reading your review, so it is `Input::html` now (I can still change it if you prefer).

igalic pievienoja piebildi

2019-08-21 20:44:37 +00:00

(Pārcelta no github.com)

definitely, what @fdb-hiroshima said

update / carification:
About implementing Ructe's ToHtml trait.

definitely, what @fdb-hiroshima said ****** update / carification: About implementing Ructe's `ToHtml` trait.

trinity-1686a pievienoja piebildi

2019-08-21 20:44:38 +00:00

Īpašnieks

I did not notice the Catalog. It was just a suggestion, but as it's totally fine as is

trinity-1686a pievienoja piebildi

2019-08-21 22:08:22 +00:00

Īpašnieks

As .html() take the Catalog, and the provided label in Input::new is always translated, can't it be translated transparently from .html() instead of explicitly each time? (I don't know if it would be better or worst, that's an actual question)

As `.html()` take the Catalog, and the provided label in Input::new is _always_ translated, can't it be translated transparently from `.html()` instead of explicitly each time? (I don't know if it would be better or worst, that's an actual question)

elegaanz pievienoja piebildi

2019-08-22 11:43:13 +00:00

(Pārcelta no github.com)

gettext-macros requires strings that are known at compile time, so that it can add them to the .pot file. With the latest version of the crate (which requires Rust 2018 for some reason, so we can't use it now), there is a new macro, called t that would allow us to do:

Input::new("email", t!("Your email address"))
    .input_type("email")
    .html(catalog);

Which would be a bit better (but we can't really do anything else than that I think, these strings should be marked as translatable with a macro at some point).

gettext-macros requires strings that are known at compile time, so that it can add them to the .pot file. With the latest version of the crate (which requires Rust 2018 for some reason, so we can't use it now), there is a new macro, called `t` that would allow us to do: ```rust Input::new("email", t!("Your email address")) .input_type("email") .html(catalog); ``` Which would be a bit better (but we can't really do anything else than that I think, these strings should be marked as translatable with a macro at some point).

trinity-1686a izskatīja 2019-08-22 13:30:44 +00:00

trinity-1686a pievienoja piebildi

Īpašnieks

👀

src/template_utils.rs

trinity-1686a pievienoja piebildi

2019-08-22 13:22:27 +00:00

Īpašnieks

I think error is no longer put in a <p class="error" dir="auto">{}</p>

I think error is no longer put in a `<p class="error" dir="auto">{}</p>`

templates/blogs/new.rs.html

					
				@ -13,0 +13,4 @@

				      		.default(&form.title)

				      		.error(&errors)

				      		.set_prop("minlength", 1)

				      		.html(ctx.1))

trinity-1686a pievienoja piebildi

2019-08-22 13:23:38 +00:00

Īpašnieks

previously had the requiredproperty

previously had the `required`property

templates/instance/admin.rs.html

					
				@ -18,3 +18,3 @@

				  <form method="post" action="@uri!(instance::update_settings)">

				    @input!(ctx.1, name (text), "Name", form, errors.clone(), "props")

				    @(Input::new("name", i18n!(ctx.1, "Name"))

trinity-1686a pievienoja piebildi

2019-08-22 13:24:39 +00:00

Īpašnieks

what was props, why is it gone?

what was `props`, why is it gone?

trinity-1686a izskatīja 2019-08-22 13:31:39 +00:00

templates/blogs/new.rs.html

					
				@ -13,0 +13,4 @@

				      		.default(&form.title)

				      		.error(&errors)

				      		.set_prop("minlength", 1)

				      		.html(ctx.1))

trinity-1686a pievienoja piebildi

2019-08-22 13:31:38 +00:00

Īpašnieks

mb, it's added automagically when not optional

elegaanz (Pārcelta no github.com) izskatīja 2019-08-22 17:17:13 +00:00

templates/instance/admin.rs.html

					
				@ -18,3 +18,3 @@

				  <form method="post" action="@uri!(instance::update_settings)">

				    @input!(ctx.1, name (text), "Name", form, errors.clone(), "props")

				    @(Input::new("name", i18n!(ctx.1, "Name"))

elegaanz (Pārcelta no github.com) pievienoja piebildi

2019-08-22 17:17:13 +00:00

I don't know to be honest, but it clearly was here by mistake (this argument was for additional HTML properties).

trinity-1686a apstiprināja šīs izmaiņas 2019-08-27 12:29:13 +00:00

trinity-1686a pievienoja piebildi

Īpašnieks

👍

Nepieciešams pieteikties, lai pievienotos šai sarunai.

Nav izskatītāju

Nav atskaites punkta

Nav projektu

Nav atbildīgo

2 dalībnieki

Paziņojumi

Izpildes datums

Izpildes datums nav derīgs vai tas ir ārpus datumu apgabala. Lūgums izmantot pierakstu "gggg-mm-dd".

Nav uzstādīts izpildes datums.

Atkarības

Nav atkarību.

Atsauce: Plume/Plume#646

Rindas
Ailes