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

Adds wallet entry to settings bar in unofficial builds #2729

Merged
merged 2 commits into from Jul 3, 2019
Merged

Conversation

@ryanml
Copy link
Member

ryanml commented Jun 18, 2019

Fixes: brave/brave-browser#4581
Fixes: brave/brave-browser#5006

is_official_build=false
Screen Shot 2019-06-18 at 1 12 16 PM

is_official_build=true
Screen Shot 2019-06-18 at 1 13 10 PM

Submitter Checklist:

Test Plan:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.
@ryanml ryanml added this to the 0.68.x - Nightly milestone Jun 18, 2019
@ryanml ryanml requested a review from bbondy Jun 18, 2019
@ryanml ryanml self-assigned this Jun 18, 2019
<path fill-rule="evenodd" d="M10 15.884c-.068 0-.136 0-.204-.003l-.928 1.413c-.24.365-.692.538-1.112.417a8.124 8.124 0 0 1-3.389-1.938.998.998 0 0 1-.194-1.175l.77-1.505a5.904 5.904 0 0 1-.197-.34l-1.705-.09a1 1 0 0 1-.922-.749 7.946 7.946 0 0 1-.244-1.946c0-.655.083-1.306.245-1.947a.998.998 0 0 1 .918-.747l1.708-.09c.062-.115.128-.229.198-.34L4.173 5.34a.998.998 0 0 1 .195-1.176 8.121 8.121 0 0 1 3.386-1.937c.422-.121.875.052 1.114.416l.928 1.414a5.872 5.872 0 0 1 .408 0l.928-1.414c.24-.364.691-.537 1.113-.416a8.126 8.126 0 0 1 3.388 1.938 1 1 0 0 1 .194 1.175l-.77 1.505c.069.111.135.225.197.34l1.705.09a1 1 0 0 1 .922.748c.16.64.244 1.29.244 1.946a7.95 7.95 0 0 1-.245 1.948.998.998 0 0 1-.918.746l-1.708.09a5.948 5.948 0 0 1-.198.34l.773 1.508a1 1 0 0 1-.197 1.174 8.124 8.124 0 0 1-3.386 1.937 1.005 1.005 0 0 1-1.114-.417l-.928-1.413a5.872 5.872 0 0 1-.204.003zm-2.087.545l1.022-1.557a.637.637 0 0 1 .592-.284 5.031 5.031 0 0 0 .946 0 .637.637 0 0 1 .592.284l1.022 1.557a6.85 6.85 0 0 0 2.513-1.435l-.849-1.655a.637.637 0 0 1 .051-.664c.183-.253.34-.52.468-.803a.637.637 0 0 1 .546-.371l1.877-.1c.105-.473.159-.952.159-1.433 0-.48-.054-.96-.159-1.433l-1.877-.1a.637.637 0 0 1-.546-.37 4.68 4.68 0 0 0-.468-.803.637.637 0 0 1-.05-.664l.848-1.656a6.853 6.853 0 0 0-2.513-1.434l-1.022 1.557a.637.637 0 0 1-.592.284 5.031 5.031 0 0 0-.946 0 .637.637 0 0 1-.592-.284L7.913 3.508A6.849 6.849 0 0 0 5.4 4.942l.849 1.656c.109.213.09.47-.051.664-.185.254-.341.522-.468.801a.637.637 0 0 1-.546.373l-1.877.1a6.635 6.635 0 0 0-.159 1.432c0 .481.054.96.159 1.433l1.877.1a.637.637 0 0 1 .546.372c.127.28.283.547.468.802.14.194.16.45.05.664L5.4 14.994a6.851 6.851 0 0 0 2.513 1.435zm.4-6.46a1.687 1.687 0 1 0 3.374 0 1.687 1.687 0 0 0-3.374 0zm-1.274 0a2.96 2.96 0 1 1 5.922 0 2.96 2.96 0 0 1-5.922 0z"/>
</svg>
</span>
<span class="nav-item_text">Wallets</span>

This comment has been minimized.

Copy link
@NejcZdovc

NejcZdovc Jun 19, 2019

Member

Probably we want to translate this? So we need to put something like [[walletTitle]] in here?

This comment has been minimized.

Copy link
@ryanml

ryanml Jun 21, 2019

Author Member

@NejcZdovc I see Brave Rewards is not translated either, I assume because all of these other translated titles were inherited from chromium definitions directly. So there'd be some patching involved to add these, not sure how that fits within the scope of the issue but happy to accommodate

This comment has been minimized.

Copy link
@ryanml

ryanml Jun 21, 2019

Author Member

Actually, I may be able to do this without patching, stay tuned

This comment has been minimized.

Copy link
@NejcZdovc

NejcZdovc Jun 21, 2019

Member

I think Brave Rewards is not translated because it's a name of the product (which we don't want to translate), where wallet is just a regular menu text like settings for example

This comment has been minimized.

Copy link
@ryanml

ryanml Jun 21, 2019

Author Member

I've used the translations with no patch, and also updated the Brave Rewards translation, separate issue created. We have string keys for it in other places, in either case (I didn't know we weren't translating it in any capacity) we should use the existing locale for it, so that has been done.

This comment has been minimized.

Copy link
@NejcZdovc

NejcZdovc Jun 21, 2019

Member

cc @davidtemkin @mandar-brave for confirmation if we want to translate Brave Rewards in the menu

This comment has been minimized.

Copy link
@davidtemkin

davidtemkin Jun 21, 2019

seems like it has to be translated -- i.e, we can't use the English string "Brave Rewards" in (for example) the Korean locale.

how is , for example, the large "Brave Rewards" at the top of the Rewards Settings page handled?

This comment has been minimized.

Copy link
@ryanml

ryanml Jun 24, 2019

Author Member

We are using a translatable string for every instance of Brave Rewards on the settings page, for consistency we should use the same here. Whether it gets translated or not with these changes we are prepared for both scenarios 👍

@ryanml ryanml requested review from NejcZdovc and davidtemkin Jun 21, 2019
@ryanml ryanml force-pushed the feature-4581 branch 2 times, most recently from a970b77 to cde010b Jun 26, 2019
@ryanml ryanml force-pushed the feature-4581 branch from cde010b to 04c14f3 Jul 2, 2019
@bbondy
bbondy approved these changes Jul 2, 2019
@ryanml ryanml modified the milestones: 0.68.x - Dev, 0.69.x - Nightly Jul 3, 2019
@ryanml ryanml merged commit f40afb3 into master Jul 3, 2019
2 checks passed
2 checks passed
continuous-integration/jenkins/pr-head This commit looks good
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@ryanml ryanml deleted the feature-4581 branch Jul 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.