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

Goals plugin language reworked #19307

Open
wants to merge 5 commits into
base: 4.x-dev
Choose a base branch
from

Conversation

comradekingu
Copy link
Contributor

Description:

Generally shorter and to the point.
In line with other edits.

Review

@justinvelluppillai
Copy link
Contributor

Thanks @comradekingu. These changes including your self-review look good. If you commit those and the fixes I've added we can merge this PR.

Co-authored-by: Justin Velluppillai <justinvelluppillai@gmail.com>
@comradekingu
Copy link
Contributor Author

@justinvelluppillai
I did all of them. Hope that was what you meant. :)

@sgiehl sgiehl added this to the 4.11.0 milestone Jun 3, 2022
@sgiehl sgiehl added the c: i18n For issues around internationalisation and localisation. label Jun 3, 2022
@justinvelluppillai justinvelluppillai added the Needs Review PRs that need a code review label Jun 7, 2022
@justinvelluppillai justinvelluppillai modified the milestones: 4.11.0, 4.12.0 Jun 7, 2022
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

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

Had a closer look through the changes and left a couple of comments for improvements.

"CaseSensitive": "Case sensitive match",
"BestCountries": "The countries with the most conversions are:",
"BestKeywords": "The keywords with the most conversions are:",
"BestReferrers": "The websites sending the most referrers are:",
Copy link
Member

Choose a reason for hiding this comment

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

This one actually now has another meaning. It should be more something like The referring websites with the most conversions are:

"ColumnConversionRatePageViewedBeforeDocumentation": "The percentage of all conversions for goal %s where this page was viewed before conversion.",
"ColumnAverageQuantityDocumentation": "The average quantity of this %s sold in e-commerce orders.",
"ColumnConversionRateDocumentation": "The percentage of visits that triggered the %s goal.",
"ColumnConversionRatePageViewedBeforeDocumentation": "The percentage of all conversions for the %s goal where this page was viewed before conversion.",
Copy link
Member

Choose a reason for hiding this comment

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

The placeholder will be filled with the name of the goal. Not sure if the change makes sense in that case.

Suggested change
"ColumnConversionRatePageViewedBeforeDocumentation": "The percentage of all conversions for the %s goal where this page was viewed before conversion.",
"ColumnConversionRatePageViewedBeforeDocumentation": "The percentage of all conversions for goal %s where this page was viewed before conversion.",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only way to do that in English is to make it
for the goal "%s", which is a bit unnatural wrt. verb, subject, object.
https://ejournals.lib.auth.gr/thal/article/view/5443 https://www.ilovelanguages.com/do-other-languages-have-inverted-verb-order-like-german/
Or "the goal of getting %s done", or something along those lines.

plugins/Goals/lang/en.json Outdated Show resolved Hide resolved
"ColumnOrdersDocumentation": "The total number of Ecommerce orders which contained this %s at least once.",
"ColumnPurchasedProductsDocumentation": "The number of purchased products is the sum of Product quantities sold in all Ecommerce orders.",
"ColumnOrdersDocumentation": "The total number of e-commerce orders which contained this %s at least once.",
"ColumnPurchasedProductsDocumentation": "The number of purchased products is the sum product quantity of all e-commerce orders sold.",
Copy link
Member

Choose a reason for hiding this comment

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

English is not my main language, but for me it sounded better like it was before. sum product quantity somehow sounds incorrect. Shouldn't that be either sum of product quantities or summed up product quantity 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sgiehl The second "Product quantities" is redundant, and "Product" should have a small "p". "Ecommerce" is also wrong.
The transitive verb "sum", like "sum total" is better than "quantities sold in all orders", as that is very dubious English.

"DaysToConv": "Days to Conversion",
"DaysToConvReportDocumentation": "This report shows how many days pass before your visitors convert a goal.",
"DaysToConvReportDocumentation": "This report shows how many days pass on average before your visitors convert a goal.",
Copy link
Member

Choose a reason for hiding this comment

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

The report actually does not show the average.
image

Suggested change
"DaysToConvReportDocumentation": "This report shows how many days pass on average before your visitors convert a goal.",
"DaysToConvReportDocumentation": "This report shows how many days pass before your visitors convert a goal.",

"HelpOneConversionPerVisit": "If a Page matching this Goal is refreshed or viewed more than once in a Visit, the Goal will only be tracked the first time the page was loaded during this visit.",
"GoalsOverview": "Overview of Goals",
"GoalsOverviewDocumentation": "An overview of your goal conversions. Initially, the graph shows the sum of all conversions. %s Below the graph, you can see conversion reports for each of your goals. The sparklines can be enlarged by clicking on them.",
"GoalX": "%s Goal",
Copy link
Member

Choose a reason for hiding this comment

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

The placeholder will also be replaced with the name of the goal

Suggested change
"GoalX": "%s Goal",
"GoalX": "Goal %s",

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should use quotes here then as well.

plugins/Goals/lang/en.json Outdated Show resolved Hide resolved
"NewVisitorsConversionRateIs": "New visitors conversion rate is %s",
"NoGoalsNeedAccess2": "Only a Write user, an Administrator or a user with Super User access can manage Goals for a given website. Please ask your Matomo administrator to set up a Goal for your website. <br>Tracking Goals is a great way to help understand and maximize your website performance!",
"ManuallyTriggeredUsingJavascriptFunction": "A goal is manually triggered using the JavaScript API trackGoal()",
"MatchesExpression": "matches the %s expression",
Copy link
Member

Choose a reason for hiding this comment

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

The placeholder contains a expression that was defined by the user like /test/

Suggested change
"MatchesExpression": "matches the %s expression",
"MatchesExpression": "matches the expression %s",

plugins/Goals/lang/en.json Outdated Show resolved Hide resolved
"SingleGoalOverviewDocumentation": "This is an overview of the conversions for a single goal. %s The sparklines below the graph can be enlarged by clicking on them.",
"ThereIsNoGoalToManage": "There is no goal to manage for website %s",
"ThereIsNoGoalToManage": "There is no goal to manage for the %s website",
Copy link
Member

Choose a reason for hiding this comment

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

The placeholder contains the name of a website...

Suggested change
"ThereIsNoGoalToManage": "There is no goal to manage for the %s website",
"ThereIsNoGoalToManage": "There is no goal to manage for website %s",

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense in German, but not in English.

Copy link
Member

Choose a reason for hiding this comment

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

Might be the reason why that feels wrong to me 🙈
Nevertheless we could consider adding quotes here as well. Wondering if it otherwise might look ugly in English if someone names his site with multiple words.

@github-actions
Copy link
Contributor

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jun 18, 2022
@sgiehl sgiehl added Waiting for user feedback Indicates the Matomo team is waiting for feedback from the author or other users. and removed Needs Review PRs that need a code review labels Jun 21, 2022
Co-authored-by: Stefan Giehl <stefan@matomo.org>
@justinvelluppillai justinvelluppillai added Needs Review PRs that need a code review and removed Waiting for user feedback Indicates the Matomo team is waiting for feedback from the author or other users. Stale The label used by the Close Stale Issues action labels Jun 23, 2022
plugins/Goals/lang/en.json Outdated Show resolved Hide resolved
"GoalConversionsBy": "Goal %s conversions by type of visit",
"GoalIsTriggered": "Goal is triggered",
"GoalIsTriggeredWhen": "Goal is triggered when",
"GoalConversionsBy": "%s-goal conversions by type of visit",
Copy link
Member

Choose a reason for hiding this comment

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

This won't work, as the goal name inserted in the placeholder might be anything. So might be better to quote it here as well.

"AddNewGoalOrEditExistingGoal": "%1$sAdd a new Goal%2$s or %3$sEdit%4$s existing Goals",
"AllowGoalConvertedMoreThanOncePerVisit": "Allow Goal to be converted more than once per visit",
"AddNewGoalOrEditExistingGoal": "%1$sAdd a new goal%2$s or %3$sEdit%4$s existing goals",
"AllowGoalConvertedMoreThanOncePerVisit": "Allow goal to be converted more than once per visit",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"AllowGoalConvertedMoreThanOncePerVisit": "Allow goal to be converted more than once per visit",
"AllowGoalConvertedMoreThanOncePerVisit": "Allow any goal to be converted more than once per visit",

Can also possibly be "a", "the", "goals", or "this"(?)
Small improvement.

@github-actions
Copy link
Contributor

github-actions bot commented Jul 2, 2022

This issue is in "needs review" but there has been no activity for 7 days. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale The label used by the Close Stale Issues action label Jul 2, 2022
"ColumnOrdersDocumentation": "The total number of Ecommerce orders which contained this %s at least once.",
"ColumnPurchasedProductsDocumentation": "The number of purchased products is the sum of Product quantities sold in all Ecommerce orders.",
"ColumnOrdersDocumentation": "The total number of e-commerce orders which contained this %s at least once.",
"ColumnPurchasedProductsDocumentation": "The number of purchased products is the sum product quantity of all e-commerce orders sold.",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
"ColumnPurchasedProductsDocumentation": "The number of purchased products is the sum product quantity of all e-commerce orders sold.",
"ColumnPurchasedProductsDocumentation": "The number of purchased products is the combined product quantity of all e-commerce orders sold.",

@github-actions
Copy link
Contributor

This PR was last updated more than one month ago, maybe it's time to close it. Please check if there is anything we still can do or close this PR. ping @matomo-org/core-reviewers

@github-actions github-actions bot added the Stale for long The label used by the Close Stale Issues action label Aug 17, 2022
@sgiehl sgiehl removed this from the 4.12.0 milestone Aug 18, 2022
@justinvelluppillai justinvelluppillai removed the Needs Review PRs that need a code review label Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: i18n For issues around internationalisation and localisation. Stale for long The label used by the Close Stale Issues action Stale The label used by the Close Stale Issues action
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants