Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add meta robots tags helper #82

Conversation

ernestas-poskus
Copy link
Contributor

Adding meta robots tags helper, I can reuse MetaProperty if needed.

Copy link
Owner

@lambda-fairy lambda-fairy left a comment

Choose a reason for hiding this comment

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

I don't think it's accurate to have two string fields, because not all strings are valid <meta robots> tags.

I think a better interface would be

struct MetaRobots {
    follow: bool,
    index: bool,
}

which the Render impl expands into the full tag.

impl<T: AsRef<str>, U: AsRef<str>> Render for MetaRobots<T, U> {
fn render(&self) -> Markup {
html! {
meta name="robots" content=(format!("{}, {}", self.0.as_ref(), self.1.as_ref())) /
Copy link
Owner

Choose a reason for hiding this comment

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

Suggest writing this as

meta name="robots" content={
    (self.0.as_ref())
    ", "
    (self.1.as_ref())
} /

since this avoids using the std::fmt machinery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

indeed, thanks for the tip !

@ernestas-poskus ernestas-poskus force-pushed the features/add_meta_robots_to_maud_extras branch from cd7529e to 93dc4bf Compare April 9, 2017 16:00
@ernestas-poskus ernestas-poskus force-pushed the features/add_meta_robots_to_maud_extras branch from 93dc4bf to 3269d7b Compare August 23, 2017 18:06
@ernestas-poskus
Copy link
Contributor Author

@lfairy hey sorry for forgetting this 1, updated according to your suggestions, for me usage now is a little cumbersome, what do you think about functoin new(index: bool, follow: bool) -> MetaRobots ? :)

@ernestas-poskus ernestas-poskus force-pushed the features/add_meta_robots_to_maud_extras branch 3 times, most recently from 4c77398 to 2ffe69b Compare August 23, 2017 18:10
@bb010g
Copy link
Contributor

bb010g commented Aug 23, 2017

I think your rebase got messed up. You may just want to reset and do a fresh commit on master.

@ernestas-poskus ernestas-poskus force-pushed the features/add_meta_robots_to_maud_extras branch from 2ffe69b to 0a28232 Compare August 23, 2017 18:23
@ernestas-poskus
Copy link
Contributor Author

I think your rebase got messed up. You may just want to reset and do a fresh commit on master.

you are right, did soft reset and new commit

let index = if self.index { "index" } else { "noindex" };
let follow = if self.follow { "follow" } else { "nofollow" };
html! {
meta name="robots" content=(format!("{},{}", index, follow)) /
Copy link
Owner

Choose a reason for hiding this comment

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

This is more efficient:

    content={ (index) "," (follow) }

@lambda-fairy
Copy link
Owner

lambda-fairy commented Aug 25, 2017

@ernestas-poskus I'm okay with adding a new method, as long as the struct fields are kept public.

@ernestas-poskus ernestas-poskus force-pushed the features/add_meta_robots_to_maud_extras branch from 0a28232 to fe4413a Compare August 25, 2017 11:29
@ernestas-poskus
Copy link
Contributor Author

ernestas-poskus commented Aug 25, 2017

fixed.

tried new

let markup = html! { (MetaRobots::new(true,false)) };

looks odd and not self explanatory left it as is

@ernestas-poskus ernestas-poskus force-pushed the features/add_meta_robots_to_maud_extras branch from fe4413a to 86050c8 Compare August 25, 2017 11:30
@lambda-fairy lambda-fairy merged commit 4d46029 into lambda-fairy:master Aug 26, 2017
@lambda-fairy
Copy link
Owner

Looks good, thanks!

@ernestas-poskus ernestas-poskus deleted the features/add_meta_robots_to_maud_extras branch August 26, 2017 05:50
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.

3 participants