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

Rename Brave Together as Brave Talk on NTP (widget) #9552

Merged
merged 2 commits into from
Jul 28, 2021
Merged

Rename Brave Together as Brave Talk on NTP (widget) #9552

merged 2 commits into from
Jul 28, 2021

Conversation

bsclifton
Copy link
Member

@bsclifton bsclifton commented Jul 26, 2021 •
edited
Loading

  • Storybook clean up (add missing items on NTP)
  • Rename Brave Together => Brave Talk (on NTP)
  • Updates widget itself, settings screen (under customize), and a few comments in the code

Fixes brave/brave-browser#16798

image
image

Submitter Checklist:

  • I confirm that no security/privacy review is needed, or that I have requested one
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
  • Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
  • Ran git rebase master (if needed)

Reviewer Checklist:

  • A security review is not needed, or a link to one is included in the PR description
  • New files have MPL-2.0 license header
  • Adequate test coverage exists to prevent regressions
  • Major classes, functions and non-trivial code blocks are well-commented
  • Changes in component dependencies are properly reflected in gn
  • Code follows the style guide
  • Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

  1. See STR under Brave Together text and image assets should read Brave Talk, in new-tab page's cards brave-browser#16798
  2. References shown in issue as Brave Together should now be Brave Talk
  3. Image (under customize) should be updated to be Brave Talk

bsclifton added 2 commits July 26, 2021 01:10
@bsclifton
Storybook clean up (add missing items on NTP)
54fdc6f
@bsclifton
Rename Brave Together => Brave Talk (on NTP)
7e00ae0 
Updates widget itself, settings screen (under customize), and a few
comments in the code

Fixes brave/brave-browser#16798
@bsclifton bsclifton self-assigned this Jul 26, 2021
@bsclifton bsclifton changed the title Rename Brave Together as Brave Talk on NTP Rename Brave Together as Brave Talk on NTP (widget) Jul 26, 2021
@bsclifton
Copy link
Member Author

Partially helps with brave/brave-browser#17138

bsclifton
bsclifton commented Jul 26, 2021
View reviewed changes
browser/ui/webui/brave_webui_source.cc
@@ -166,7 +166,6 @@ void CustomizeWebUIHTMLSource(const std::string &name,
{ "showFavoritesMenuLabel", IDS_BRAVE_NEW_TAB_SHOW_FAVORITES_MENU_LABEL }, // NOLINT
{ "showRewards", IDS_BRAVE_NEW_TAB_SHOW_REWARDS },
{ "showBinance", IDS_BRAVE_NEW_TAB_SHOW_BINANCE },
{ "showTogether", IDS_BRAVE_NEW_TAB_SHOW_TOGETHER },
Copy link
Member Author

@bsclifton bsclifton Jul 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This is no longer used; I think it may be from when we shared this preference on brave://settings/newTab

bsclifton
bsclifton commented Jul 26, 2021
View reviewed changes
components/resources/brave_components_strings.grd
@@ -221,7 +221,6 @@

<message name="IDS_BRAVE_NEW_TAB_SHOW_REWARDS" desc="Text for settings show rewards option">Show Brave Rewards</message>
<message name="IDS_BRAVE_NEW_TAB_SHOW_BINANCE" desc="Text for show binance option">Show Binance</message>
<message name="IDS_BRAVE_NEW_TAB_SHOW_TOGETHER" desc="Text for show together option">Brave Together</message>
Copy link
Member Author

Choose a reason for hiding this comment

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

See https://github.com/brave/brave-core/pull/9552/files#r676397520

simonhong
simonhong approved these changes Jul 27, 2021
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++

stephendonner
stephendonner approved these changes Jul 27, 2021
View reviewed changes
Copy link
Contributor

@stephendonner stephendonner left a comment

Choose a reason for hiding this comment

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

LGTM; thanks!

@bsclifton
Copy link
Member Author

All platforms passed 😄 macOS had a CI warning on JUnit test (failing on Archive JUnit-formatted test results). Going to merge!

@bsclifton bsclifton merged commit e8cf3d2 into master Jul 28, 2021
@bsclifton bsclifton deleted the bsc-brave-talk-rename branch July 28, 2021 06:22
@bsclifton bsclifton added this to the 1.29.x - Nightly milestone Jul 28, 2021
bsclifton added a commit that referenced this pull request Aug 2, 2021
@bsclifton
Merge pull request #9552 from brave/bsc-brave-talk-rename
5701eca 
Rename Brave Together as Brave Talk on NTP (widget)
@bsclifton bsclifton mentioned this pull request Aug 2, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stephendonner stephendonner stephendonner approved these changes

@simonhong simonhong simonhong approved these changes

@petemill petemill Awaiting requested review from petemill

@nullhook nullhook Awaiting requested review from nullhook

Assignees

@bsclifton bsclifton

Labels
None yet
Projects
None yet
Milestone
1.29.x - Release
Development

Successfully merging this pull request may close these issues.

Brave Together text and image assets should read Brave Talk, in new-tab page's cards
3 participants
@bsclifton @stephendonner @simonhong