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

Updates footer link name #1337

Merged
merged 3 commits into from
Apr 12, 2017
Merged

Updates footer link name #1337

merged 3 commits into from
Apr 12, 2017

Conversation

esgoodman
Copy link
Contributor

Why Because the previous version was inaccurate; the linked page now contains security policy information along with privacy information.

**Why** Because the past name was inaccurate; the linked page now contains security policy information along with privacy information.
@@ -12,7 +12,7 @@ en:
auth_app_fallback_html: ' or %{link}.'
fallback_to_sms_html: Send me a text message with the code instead
fallback_to_voice_html: If you can't get text message right now, you can get a security code via %{link}
privacy_policy: Privacy Policy
privacy_policy: Privacy & security
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use and instead of the ampersand?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could - the ampersand was intended to save a little space on the mobile version of the footer. Say more about why you'd like the full word!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh -- did I forget to escape-ify it?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes the & needs HTML-escaping if we use it. I've seen mixed use on the site, and I personally like the longhand better.

Copy link
Contributor

Choose a reason for hiding this comment

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

these things are auto HTML-escaped, so don't worry about escaping that here

Copy link
Contributor Author

@esgoodman esgoodman Apr 7, 2017

Choose a reason for hiding this comment

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

@jeanninehunter Your guidance would be appreciated! & or and ?

Copy link
Contributor

@jeanninehunter jeanninehunter Apr 7, 2017

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pkarman Done! Changed to and. Since @jeanninehunter has lowered the gavel, that is now our style everywhere.

**Why** Because that's our style.
@hursey013
Copy link
Member

@esgoodman this looks like it just needs a test adjusted to reflect the new link text - want me to update it for you?

@esgoodman
Copy link
Contributor Author

Please do @hursey013! I always forget to change the test.

Copy link
Contributor

@zachmargolis zachmargolis left a comment

Choose a reason for hiding this comment

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

LGTM

@zachmargolis zachmargolis merged commit d218664 into master Apr 12, 2017
@zachmargolis zachmargolis deleted the eg-footer-text branch April 12, 2017 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants