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.

Sign up for GitHub

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

Jump to bottom

Fixes Issue #12421 - Change page title based on the site section being viewed #12627

Merged
merged 21 commits into from Oct 26, 2020
Merged

Fixes Issue #12421 - Change page title based on the site section being viewed #12627

merged 21 commits into from Oct 26, 2020

Conversation

jbusa22
Copy link
Contributor

@jbusa22 jbusa22 commented Sep 28, 2020 •
edited

Fixes #12421

Changes

Added dynamic title updating by modifying the title attribute in the vue store on component mount.
Current dynamic title sections are:
Tasks
Inventory
Shops
Party
Guilds
Group
Challenges

UUID: 7f566b3a-3b43-4035-b712-8ddef3e82a71

@Alys Alys changed the title Fixes Issue #12421 Fixes Issue #12421 - Change page title based on the site section being viewed Oct 5, 2020
@paglias
Copy link
Contributor

paglias commented Oct 5, 2020

@jbusa22 I tested the PR and overall it's working well, there are a few issues though:

  1. If you edit the name of a guild / party / challenge the name in the title doesn't change until the page is refreshed. A solution might be do add a watcher (https://vuejs.org/v2/guide/computed.html#Watchers) on the group / challenge name and update the title when that changes
  2. Looks like the groupPlans string already exists
 Error: groupPlans in ./website/common/locales/en/subscriber.json already exists in ./website/common/locales/en/groups.json.
  1. When switching to a page that doesn't have a title like the Settings the name is not reset to Habitica
  2. For a group plan page we should use NAME | Group Plans | Habitica

And a couple of things for @Tressley

  1. In the issue I noticed you used Teams in place of Groups Plans but the wording across the site has not yet been changed to Teams
  2. Do we want to add a few more titles here? I noticed that we're not covering: PMs, User's profile / stats / achievement pages in the user modal, settings

@Tressley
Copy link
Collaborator

Tressley commented Oct 5, 2020

@paglias

In the issue I noticed you used Teams in place of Groups Plans but the wording across the site has not yet been changed to Teams

Yeah, we're not settled on a date for that change, so let's move forward with "Group" until we're ready to move forward on the potential name change.

Do we want to add a few more titles here? I noticed that we're not covering: PMs, User's profile / stats / achievement pages in the user modal, settings

Yeah, I think this should try to hit all sections on the site. I outlined most of them, but was sure I was missing others. For the sections I missed, they should follow the same structure: Sub-section | Section | Habitica

@jbusa22
Copy link
Contributor Author

jbusa22 commented Oct 5, 2020

In response to @paglias,

  1. Sounds good I will add watchers and test name changing to the group, guild / party, and challenge sections
  2. Easy fix
  3. There doesn't seem to be a nice way to check this. If we have all pages outlined here then this won't be a problem so maybe this is okay?
  4. I'm a little confused about this. Do you mean on your group's page it should be NAME | Group Plans | Habitica and on the actual group plans page just Group Plans | Habitica?

@paglias
Copy link
Contributor

paglias commented Oct 5, 2020

@jbusa22

  1. I think the merge commit reverted the previous one where the string was removed
  2. yes once we have all pages outline it won't be a problem, I'd also add a comment in the router file to explain that each page will have to set its title
  3. I mean in website/client/src/components/groups/group.vue it's just setting the section but not subSection, I think it should be group.name | Group Plans | Habitica. Does it make sense?

@jbusa22
Copy link
Contributor Author

jbusa22 commented Oct 5, 2020

@paglias Yes, that makes sense.

@jbusa22
Copy link
Contributor Author

jbusa22 commented Oct 19, 2020

I think I hit all of the sections / subsections. I also added a watcher like you said to the challenges, but just used an already defined function for groups. Also features isn't used in the json, so I added it, not sure if I need to do anything else for translations to work.

paglias
paglias suggested changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@paglias paglias left a comment

Choose a reason for hiding this comment

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

thanks @jbusa22 , I left 3 small comments then it should be good to go :)

website/client/src/components/groups/group.vue Outdated Show resolved Hide resolved
website/client/src/components/challenges/challengeDetail.vue Outdated Show resolved Hide resolved
website/client/src/router/index.js Outdated Show resolved Hide resolved
paglias
paglias reviewed Oct 20, 2020
View reviewed changes
website/client/public/index.html Outdated
@@ -4,7 +4,7 @@
<meta charset="utf-8">
<meta http-equiv="X-UA-Compatible" content="IE=edge">
<meta name="viewport" content="width=device-width, initial-scale=1">
<title>Habitica - Gamify Your Life</title>
<title>Habitica</title>
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry, Gamify Your Life can stay in this file

paglias
paglias suggested changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@paglias paglias left a comment

Choose a reason for hiding this comment

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

Did another round of testing, left some comments, other than that the only two things missing are titles for:

  • Login
  • Register

website/common/locales/ach/groups.json Outdated
@@ -14,6 +14,7 @@
"askQuestionGuild": "Ask a Question (Habitica Help guild)",
"contributing": "Contributing",
"faq": "FAQ",
"features": "Features",
Copy link
Contributor

Choose a reason for hiding this comment

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

@jbusa22 adding the string is the correct thing to do but it should be added only to the locales/en/groups.json file

paglias
paglias suggested changes Oct 20, 2020
View reviewed changes
website/common/locales/en/groups.json Show resolved Hide resolved
website/common/locales/en/front.json Show resolved Hide resolved
website/client/src/components/auth/registerLoginReset.vue Outdated Show resolved Hide resolved
website/client/src/components/static/features.vue Outdated Show resolved Hide resolved
website/client/src/components/userMenu/profile.vue Outdated Show resolved Hide resolved
website/client/src/components/static/home.vue Outdated Show resolved Hide resolved
@jbusa22 jbusa22 requested a review from paglias October 24, 2020 16:19
@paglias paglias merged commit ad51675 into HabitRPG:develop Oct 26, 2020
@paglias
Copy link
Contributor

paglias commented Oct 26, 2020

@jbusa22 thanks for this PR! I'm merging the PR and awarding your contributor tier.

Note that further tiers require increasing amounts of work from one to the next, so Tier 2 may require a couple of PRs or a larger PR to attain. But keep helping out and we'll express our gratitude accordingly!

@paglias paglias mentioned this pull request Oct 26, 2020
Closed
This was referenced Oct 30, 2020
Closed
Closed
bakerty pushed a commit to bakerty/habitica that referenced this pull request Dec 18, 2020
@jbusa22 @paglias @bakerty
Fixes Issue HabitRPG#12421 - Change page title based on the site sect…
1dfa9fe 
…ion being viewed (HabitRPG#12627)

* Update title for tabs not including challenges, guild and team

* add section titles to challenges, guilds, and groups

* Update dynamic title to use vuex action

* Remove duplicate key

* Actually remove duplicate key

* Fix section sub section in group

* Add note to implement setTitle when adding a page

* Add missing sections to dynamic title

* Features string not translated

* Use onGroupUpdate to update group titles

* Add watcher to challenges for dynamic title updates

* Small fixes

* Add register and login to title, remove duplicate keys

* Add home page dynamic title functionality

* Minor name changes

* remove wrong i18n strings from front.js

* refactor router note

Co-authored-by: Matteo Pagliazzi <matteopagliazzi@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@paglias paglias paglias requested changes

@benkelaar benkelaar benkelaar left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Change page title based on the site section being viewed
5 participants
@jbusa22 @paglias @Tressley @benkelaar @SabreCat