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

issue 948 - title tags update #967

Merged
merged 3 commits into from
Nov 7, 2023

Conversation

ice1080
Copy link
Contributor

@ice1080 ice1080 commented Oct 3, 2023

For issue #948
Open to suggestions on better ways to do this. I'm fairly new to ruby.

Copy link
Member

@rudokemper rudokemper left a comment

Choose a reason for hiding this comment

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

Just one small fix! Thank you.

@@ -1,7 +1,7 @@
<!DOCTYPE html>
<html>
<head>
<title>Terrastories</title>
<title>Terrastories - <%= @community.name %></title>
Copy link
Member

Choose a reason for hiding this comment

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

Set this to Terrastories: <%= @community.name %></title>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed this update!

@@ -1,7 +1,7 @@
<!DOCTYPE html>
<html>
<head>
<title>Terrastories</title>
<title>Welcome - Terrastories<%= @community ? ': ' + @community.name : '' %></title>
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's use interpolation rather than string concatenation and check for community name in addition to community existence:

Suggested change
<title>Welcome - Terrastories<%= @community ? ': ' + @community.name : '' %></title>
<title>Welcome - Terrastories<%= @community&.name ? ": #{@community.name}": '' %></title>

In particular, this will ensure that if community exists but name is somehow nil, it won't crash the page.

irb> ': ' + nil
TypeError (no implicit conversion of nil into String)
irb> ": #{nil}"
=> ": "

@rudokemper Do we also want to localize "Welcome" ?

Copy link
Member

Choose a reason for hiding this comment

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

Great suggestions @lauramosher and yes - that would be a welcome 🤦 change.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed the nil safe and interpolation update, as well as localization!

@lauramosher lauramosher merged commit 8365125 into Terrastories:master Nov 7, 2023
@ice1080 ice1080 deleted the 948-title-tag-update branch November 14, 2023 01:53
ice1080 added a commit to ice1080/terrastories that referenced this pull request Nov 14, 2023
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.

3 participants