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

Updated new-groups / group-plans static page #8674 #8729

Merged
merged 11 commits into from Jul 6, 2017

Conversation

Projects
None yet
4 participants
@yugensoft
Copy link
Contributor

yugensoft commented May 13, 2017

Fixes #8674

Changes

The group-plans pages (both static and in /options/groups) have been updated to match the drawing in the issue.
To meet the requirements of this issue, the following was done:

  • The new group creation state was moved out to it's own URL, to both the /static/plans and /options/groups/group-plans routes can link to it
  • The .jade content files were updated to the desired design, including moving common content out to mixins in new files
  • The groupBenefit i18n strings were renamed (by script over the whole locales directory) to descriptive names, making reading of the corresponding .jade files easier
  • A static/login page was added, in addition to the existing modal login, with both now using the original modal login body code. This allows a smooth login & redirect if the user clicks the Create Group button from the /static/plans page whilst they are not logged in, or if they aren't registered yet.

UUID: 6365790b-b0ea-4da6-bbe3-f5a217330ad2

yugensoft added some commits Apr 23, 2017

Added redirect-through-login functionality from the static/plans page…
… new-group button

This includes a static non-modal login page (similar to how other sites have both a login page and login modal)
The login body has been abstracted out from its modal-specific view into mixins to accomplish this
@TheHollidayInn

This comment has been minimized.

Copy link
Collaborator

TheHollidayInn commented May 23, 2017

Thanks for the PR! You only need to edit the english versions. All other versions are handled via a 3rd party system. Can you remove those changes?

@yugensoft

This comment has been minimized.

Copy link
Contributor

yugensoft commented May 23, 2017

But they are renamings of the keys (e.g. groupBenefitOneTitle to groupBenefitSharedTaskTitle), not changes in the text-to-display. These new descriptive names are to allow the content/element positioning in the jade files in question to be immediately readable to the developer. My understanding is that if you rename keys (not just change the text), you have to do it project-wide (http://habitica.wikia.com/wiki/Guidance_for_Blacksmiths#Modifying_Translatable_Strings). Is this not the case?

@Alys

This comment has been minimized.

Copy link
Contributor

Alys commented May 23, 2017

@yugensoft I'm very sorry, I think that text in the wiki page might be misleading, in which case we'll get it changed. Whenever you change translatable strings, the locales/en/ directory is the only one you should change; all the others are handled in Transifex. In brief, only the /en/ files are copied from GitHub to Transifex, and then the other locales files are copied in the reverse direction.

Was it this paragraph that mislead you?
"Changing the text of a translatable string is acceptable, but the keys can be referenced elsewhere in Habitica's code. If the key is changed in one place, all the other locations would need to be updated as well."
"all the other locations" was meant to refer to locations in the code rather than in the locales files, although reading it now I realise that's not at all clear. :( I'll edit it..

If it was a different part of that page that was misleading, please tell us which part!

@yugensoft

This comment has been minimized.

Copy link
Contributor

yugensoft commented May 23, 2017

OK I see (yes I took 'all other locations' literally), but that would imply they will have to re-translate any strings which stay the same but have their key names changed, right? Well given this is how it works I'll remove the changes to the non en directories.

Added form link (#8674 (comment))
Removed changes to non-EN locale files (#8729 (comment))
@yugensoft

This comment has been minimized.

Copy link
Contributor

yugensoft commented May 23, 2017

OK it's done

@Alys

This comment has been minimized.

Copy link
Contributor

Alys commented May 24, 2017

they will have to re-translate any strings which stay the same but have their key names changed, right?

Yes, that's right. Because of that, I'm in two minds about the changes to the keys. It will make the jade file easier to edit if it needs editing again but it means that all the translations need to be redone, and Transifex won't show the original translations in a convenient way (or might not even show them anywhere at all since they're being deleted).

Could you please show us screenshots of what the two pages look like now?

@yugensoft

This comment has been minimized.

Copy link
Contributor

yugensoft commented May 24, 2017

group-plans
static

Both are zoomed out as far as it takes to screenshot all the content.

@yugensoft

This comment has been minimized.

Copy link
Contributor

yugensoft commented May 24, 2017

On the strings, is there no way of changing keys/identifiers in transifex?

@Alys

This comment has been minimized.

Copy link
Contributor

Alys commented May 28, 2017

@yugensoft I believe there's no way to change keys in transifex.

@yugensoft

This comment has been minimized.

Copy link
Contributor

yugensoft commented May 28, 2017

@Alys OK, I'll revert the key names and just put a comment next to each one

@yugensoft

This comment has been minimized.

Copy link
Contributor

yugensoft commented May 28, 2017

OK it's done

$rootScope.clickContact = function(){
Analytics.track({'hitType':'event','eventCategory':'button','eventAction':'click','eventLabel':'Contact Us (Plans)'})
}
};
$rootScope.goToNewGroupPage = function () {

This comment has been minimized.

@TheHollidayInn

TheHollidayInn Jun 6, 2017

Collaborator

Why is this added the the $rootscope?

This comment has been minimized.

@yugensoft

yugensoft Jun 7, 2017

Contributor

You're right it can just be $scope

This comment has been minimized.

@TheHollidayInn

TheHollidayInn Jun 7, 2017

Collaborator

Awesome, can you change that, please.

*/

angular.module('habitrpg')
.controller("NewGroupCtrl", ['$scope', '$window', 'Groups', 'Payments',

This comment has been minimized.

@TheHollidayInn

TheHollidayInn Jun 6, 2017

Collaborator

Is this new controller needed?

This comment has been minimized.

@yugensoft

yugensoft Jun 7, 2017

Contributor

I was just following precedent in that new pages seem to always get new controllers, e.g. from habitica/website/client-old/js/app.js:129 onwards, all the "social" sub-states have their own template pages and their own controller.
The original page needed to be split into two because its 'create group' sub-section is now being linked into from two locations (a 'static' front page where the user may be outside the login wall), and the internal group plans page.

This comment has been minimized.

@TheHollidayInn

TheHollidayInn Jun 7, 2017

Collaborator

It seems to be that you don't use the create group section in the static. Don't you do a redirect?

This comment has been minimized.

@yugensoft

yugensoft Jun 7, 2017

Contributor

Sorry could you rephrase?
In the original groupPlansCtrl.js, it had 3 consecutive sections (for search of a better word, each 'section' being hidden/shown using the $scope.activePage). These were BENEFITS -> CREATE_GROUP (where you enter the group details) -> UPGRADE_GROUP (which really means like "pay for right to make groups").
The original static page was something completely different (now discarded)
The content of BENEFITS is now shown in the /static/plans and the /#/options/groups/group-plans, using a mixin. But both need to send the user to the inside-the-login CREATE_GROUP page (directly or indirectly) so they can enter the group details and make it.
This leaves the original groupPlansCtrl.js just a husk, but it's possible /static/plans and /#/options/groups/group-plans deviate in the future so I didn't try to cull it.

@TheHollidayInn

This comment has been minimized.

Copy link
Collaborator

TheHollidayInn commented Jun 6, 2017

Looks good to me! and works well locally.

I just had a few comments about some code choices

@SabreCat SabreCat merged commit c502b19 into HabitRPG:develop Jul 6, 2017

@SabreCat

This comment has been minimized.

Copy link
Member

SabreCat commented Jul 6, 2017

Thanks @yugensoft! I've noted this in your progress toward Blacksmith contributor tier 3.

@ghost

This comment has been minimized.

Copy link

ghost commented Jul 30, 2017

@SabreCat a question from a translator on Transifex.

Please excuse me if it's not an appropriate place to ask, but it seems that this PR has introduced the mailto button to send messages to @veeeeeee regarding the group plans.

The subject enterprisePlansEmailSubject of emails sent from the static plans page is translatable. So we just want to double-check it with you, that it's intentional (and for example you don't rely on email filtering for messages with the unified subject).

@SabreCat

This comment has been minimized.

Copy link
Member

SabreCat commented Jul 30, 2017

@GitHubSphinx: Oh, good eye, I did not think of that! You are correct, that should be a static string, not an internationalized one.

@ghost ghost referenced this pull request Aug 2, 2017

Closed

Translators suggest minor changes to some locale strings #8917

1 of 5 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment