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

Fix promoted card url #9816

Closed
wants to merge 3 commits into from
Closed

Conversation

abhiklodh
Copy link

@abhiklodh abhiklodh commented Aug 22, 2021

Promoted news card now loads https://brave.com/brave-news.

Resolves brave/brave-browser#17584

After enabling Brave News, scrolling down to a promoted article and hovering on the 'Promoted' tag on the bottom right, https://brave.com/brave-today appears as the URL and clicking on it leads to the article page.

I went through the code and changed the promotedInfoUrl to https://brave.com/brave-news and made sure that simply clicking on the Promoted tag leads to https://brave.com/brave-news instead of the article.

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:

A detailed test plan is described in the issue.

@abhiklodh

This comment has been minimized.

@abhiklodh
Copy link
Author

abhiklodh commented Aug 23, 2021

My new commit now fixes the original issue.
// TODO(petemill): Avoid nested links
@petemill @simonhong could you please review my fix and give some feedback?

@simonhong
Copy link
Member

@nullhook Can you take a look this PR?

@nullhook
Copy link
Contributor

Can you please squash your commits to one so I can review?

Promoted news card now loads brave.com/brave-news

Promoted tag now opens brave.com
@abhiklodh
Copy link
Author

@nullhook I fetched the latest commits from brave/brave-core and then squashed all my commits in one. Hope this helps.

@abhiklodh
Copy link
Author

@nullhook I have had severe storage issues that are preventing me from further maintaining the entire codebase on my work computer. Hence I had to shift my repository to my laptop and I am facing Git issues that are not letting me squash my commits.
Could you kindly review my code after squashing the commits?

@bsclifton
Copy link
Member

Closing - apologies we didn't get a chance to properly review. We've removed the promoted functionality and brave/brave-browser#17584 has been closed

@bsclifton bsclifton closed this Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Promoted news card loads the landing page rather than what its linked to
4 participants