Skip to content

Conversation

@ajayseeker
Copy link
Contributor

@ajayseeker ajayseeker commented Aug 14, 2024

Resolves :- brave/brave-browser#39167
It moves the Add button to a more appropriate place.

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:

@ajayseeker
Copy link
Contributor Author

Hi @bsclifton and @fallaciousreasoning!
Can you please assign me this issue?
I've figured out the way to introduce separator post brave icon in the location bar, working on the cursor related issue.

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.

Thanks for working this out!

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.

Comment - this looks great! If you fix the (tiny) nit I'll kick off CI and I think we can get this merged!

@ajayseeker
Copy link
Contributor Author

Comment - this looks great! If you fix the (tiny) nit I'll kick off CI and I think we can get this merged!

Corrected

@fallaciousreasoning
Copy link
Contributor

fallaciousreasoning commented Aug 20, 2024

Hey @ajayseeker I noticed the first commit isn't signed - could you sign it or squash the commits (we won't be able to merge, even if CI passes unless they're all signed).

I made a separate PR with your changes to kick of CI
#25237

*edit: oh no, looks like there's a new check that the PR is based on the most recent Chromium version 😨 You'll probably need to rebase on latest master to get it to pass.

*edit: looks like presubmit failed too - could you run npm run presubmit -- --fix and fix all the output - its a bit fiddly and often only checks committed changes.

@fallaciousreasoning
Copy link
Contributor

@ajayseeker if you need a hand setting up commit signing, let me know!

@ajayseeker
Copy link
Contributor Author

Hey! sorry! I'll not be able to work on this.
I'm un-assigning myself.

@ajayseeker ajayseeker removed their assignment Nov 1, 2024
@mihaiplesa mihaiplesa requested a review from a team as a code owner November 4, 2025 12:16
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.

2 participants