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

Fix translation issues on PDP and RTL placement for Tooltips #2642

Merged
merged 12 commits into from
Aug 15, 2017
Merged

Fix translation issues on PDP and RTL placement for Tooltips #2642

merged 12 commits into from
Aug 15, 2017

Conversation

kieckhafer
Copy link
Member

@kieckhafer kieckhafer commented Aug 4, 2017 •
edited
Loading

Resolves #2641
Resolves #2287

Divider
The divider component is registering as pure.

As of right now, it cannot be a pure component, as the i18nKey never changes, even with a language switch, it's still an i18nKey like app.public. Since the key stays the same, the props don't change, the component doesn't update.

Tooltips
Tooltips were overflowing outside the window area.

I changed the tether to use window instead of the current element, and that seems to have fixed it everywhere.

Remaining Translations
[WIP] please help i don't know.

@kieckhafer
Copy link
Member Author

kieckhafer commented Aug 4, 2017 •
edited
Loading

  • Tooltips are fixed.
  • Translations on PDP are fixed.
  • Still having issues with a few of the panels in the dashboard, and I am struggling to figure it out, because it seems very random which ones don't work. Some are a new React panel title, some are older Blaze templates, but they all have available translations, and other React / Blaze templates with the exact same setup are working.

These three are the remaining culprits, if anyone has any ideas... help please!

reaction
reaction 2
x_revolve_stanton_top

@kieckhafer kieckhafer mentioned this pull request Aug 4, 2017
Closed
@kieckhafer kieckhafer changed the title [WIP] Fix translation issues on PDP and RTL placement for Tooltips Fix translation issues on PDP and RTL placement for Tooltips Aug 8, 2017
@brent-hoover
Copy link
Collaborator

@kieckhafer Is this ready to go? Should we just merge the fixes you have in and open issues for the few remaining?

@kieckhafer
Copy link
Member Author

The above issue are still there, but I think we should push and make a new ticket. AFAIK all translations except these three will be fixed with this update.

@kieckhafer kieckhafer mentioned this pull request Aug 14, 2017
Closed
@kieckhafer
Copy link
Member Author

Here's a ticket for the rest: #2672

@brent-hoover
Copy link
Collaborator

@kieckhafer Do you want to assign reviewers?

@brent-hoover
Copy link
Collaborator

Ok, I guess I deserved that

@brent-hoover
Copy link
Collaborator

It looks like "Dashboard" is still untranslated. Or that could be just a missing translation? Is there a certain language I should test with.

cart_checkout

@aaronjudd
Copy link
Contributor

You'll need to run lingohub and get all the translations for the new coreTitle..

@brent-hoover
Copy link
Collaborator

How do I create translations from a branch again?

@aaronjudd
Copy link
Contributor

aaronjudd commented Aug 15, 2017 •
edited
Loading

repository_setting___lingohub
But I see you just figured it out.. but you can push to the PR now...

LingoHub Update 🚀
f0229cc 
Manual push by LingoHub User: Brent Hoover.
Project: reaction

Made with ❤️ by https://lingohub.com
@brent-hoover
Copy link
Collaborator

"Dashboard" still untranslated

kieckhafer and others added 2 commits August 14, 2017 19:30
@kieckhafer
add coretitle translation
a7517ed
LingoHub Update 🚀
dd7c4bb 
Manual push by LingoHub User: Brent Hoover.
Project: reaction

Made with ❤️ by https://lingohub.com
@brent-hoover
Copy link
Collaborator

"Dashboard" is now translated.

@brent-hoover
Copy link
Collaborator

Looks like all the translations in the variant panel are missing

basic_reaction_product

Look to be translated in master.

basic_reaction_product

@brent-hoover
Copy link
Collaborator

I'm not sure what the best course of action here, to keep testing this or just merge and open new issues. I think this fixes the specific issues pointed out in ticket?

brent-hoover
brent-hoover approved these changes Aug 15, 2017
View reviewed changes
Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Fixes bugs as outlined in ticket, although there are still translation issues in the PDP.

@brent-hoover brent-hoover merged commit f570545 into marketplace Aug 15, 2017
@brent-hoover brent-hoover deleted the ek-issue2641 branch August 15, 2017 03:33
jshimko added a commit that referenced this pull request Aug 15, 2017
@jshimko
Merge branch 'marketplace' into register-i18n-components
cf4bc7b 
* marketplace:
  Fix for errors while viewing orders made with Braintree (#2659)
  Fix translation issues on PDP and RTL placement for Tooltips (#2642)

# Conflicts:
#	imports/plugins/core/ui/client/components/divider/divider.js
brent-hoover added a commit that referenced this pull request Aug 18, 2017
@brent-hoover
Revert "Fix translation issues on PDP and RTL placement for Tooltips (#…
2bd2836 
…2642)"

This reverts commit f570545.
kieckhafer
kieckhafer commented Aug 18, 2017
View reviewed changes
imports/plugins/core/accounts/server/i18n/bg.json
"accountsSettingsLabel": "Профили",
"accountsSettingsTitle": "Профили",
"accountsSettingsDescription": "Настройки на профила"
},
"groupsInvite": {
"name": "Джон Смит",
"email": "[email protected]"
"email": "[email protected]",
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we change this to a non-reaction email on the en version??

Copy link
Contributor

Choose a reason for hiding this comment

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

yes should be email@localhost -> and should not be translated in LingoHub (save as a phrase).

@spencern spencern mentioned this pull request Oct 11, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aaronjudd aaronjudd aaronjudd left review comments

@brent-hoover brent-hoover brent-hoover approved these changes

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

Successfully merging this pull request may close these issues.
4 participants
@kieckhafer @brent-hoover @aaronjudd @mikemurray