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: prevent parent element onClick from triggering #24219

Merged

Conversation

brandedcow
Copy link
Contributor

Resolves brave-browser#36550

Summary

  • Added preventDefault on button to prevent parent element onClick from triggering

Depends On
Updated button props

Submitter Checklist:

  • I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
  • There is a ticket for my issue
  • Used Github auto-closing keywords in the PR description above
  • Wrote a good PR/commit description
  • Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
  • 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 wiki
    • npm run presubmit wiki, 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:

Copy link
Contributor

@fallaciousreasoning fallaciousreasoning left a comment

Choose a reason for hiding this comment

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

This looks good. Once brave/leo#713 lands I'll update it in brave-core and we can kick of CI for this PR.

Thanks @brandedcow! (note: you'll need to sign this commit too)

@fallaciousreasoning
Copy link
Contributor

Hey @brandedcow I don't think the commit before the merge is signed. I would do the following to resign and avoid needing a merge commit:

git reset --hard HEAD~1 # reset before the merge commit
git rebase brave/master # rebase onto master

because the rebase modifies the commit it should be signed. If not you can do a git commit --amend --no-edit -S

@brandedcow brandedcow closed this Jun 25, 2024
@brandedcow brandedcow force-pushed the fix/news-collapsing-side-panel branch from e522f42 to 00f86a7 Compare June 25, 2024 23:28
@brandedcow brandedcow reopened this Jun 25, 2024
@brandedcow
Copy link
Contributor Author

@fallaciousreasoning Let me know if the commit is properly signed, I reset my branch to head and made a signed commit.

@fallaciousreasoning
Copy link
Contributor

Sweet, that looks good - unfortunately it'll be a while until we get this one merged 'cause I'll need to bump Nala first (and that's never very quick).

@fallaciousreasoning
Copy link
Contributor

@brandedcow once #24379 lands you'll need to rebase (it'll probably take a few days at least) and I can kick off CI.

Sorry its such a process 😆

@fallaciousreasoning
Copy link
Contributor

Mind rebasing?

@brandedcow brandedcow force-pushed the fix/news-collapsing-side-panel branch 3 times, most recently from 856416c to e28a484 Compare July 1, 2024 16:57
@brandedcow brandedcow force-pushed the fix/news-collapsing-side-panel branch from e28a484 to 4477180 Compare July 1, 2024 16:59
@brandedcow
Copy link
Contributor Author

@fallaciousreasoning Just updated the PR!

@brandedcow
Copy link
Contributor Author

@fallaciousreasoning Hey, just checking in on this PR, I see one of the checks were unsuccessful and it is blocking the merge, do I need to do anything on my end?

@fallaciousreasoning
Copy link
Contributor

fallaciousreasoning commented Jul 8, 2024

Whoops forgot about this. I'll just see if I can work out why CI isn't passing. Kicked off CI properly

@fallaciousreasoning fallaciousreasoning merged commit 631efd6 into brave:master Jul 9, 2024
24 of 25 checks passed
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.

[Brave News]: Publisher list in side panel collapses after adding/updating publishers/channels
2 participants