Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Removed ntp tile outline #8327

Merged
merged 1 commit into from
Mar 25, 2021
Merged

Removed ntp tile outline #8327

merged 1 commit into from
Mar 25, 2021

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Mar 23, 2021

fix brave/brave-browser#14802

Resolves
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:

@simonhong simonhong requested review from petemill and bsclifton March 23, 2021 06:25
@simonhong simonhong self-assigned this Mar 23, 2021
@bsclifton
Copy link
Member

I don't think we need this anymore - looks fixed w/ Chromium 90. @simonhong can you verify? Tiles PR was in 1.23 which will also feature Chromium 90

@simonhong simonhong force-pushed the remove_ntp_tile_outline branch from 698761c to 74b46be Compare March 24, 2021 23:35
@simonhong
Copy link
Member Author

simonhong commented Mar 24, 2021

@bsclifton I confirmed that latest nightly (c90) still has this issue.

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Works great! 😄

@simonhong simonhong added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Mar 25, 2021
@simonhong
Copy link
Member Author

re-run macos CI only. others are fine with previous run.

@bsclifton
Copy link
Member

Looking at the change a bit further (and trying things with dev tools). The right kind of fix might be to instead specify outline:none for the anchor tag. unset does work but feels like when you want it to fall back to default (versus not show at all)

@simonhong simonhong force-pushed the remove_ntp_tile_outline branch from 74b46be to 9436972 Compare March 25, 2021 06:48
@simonhong simonhong removed CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/skip-linux labels Mar 25, 2021
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Perfect 😄

edit: found a problem maybe 10 seconds after approving lol
image

I think the first line edited (removal of unset) should be kept. I think we just need the 2nd line of change

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

see comment (my bad)

@simonhong simonhong force-pushed the remove_ntp_tile_outline branch from 9436972 to 8939b70 Compare March 25, 2021 07:07
@simonhong simonhong requested a review from bsclifton March 25, 2021 07:07
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Looks great! 😄
image

@simonhong
Copy link
Member Author

Two below browser tests are failed only on linux and both are definitely not related with this PR's change. Merged.
BraveNavigatorPluginsFarblingBrowserTest.FarbleNavigatorPluginsReset
RewardsContributionBrowserTest.AutoContributionMultiplePublishersUphold

@simonhong simonhong merged commit 55a323b into master Mar 25, 2021
@simonhong simonhong deleted the remove_ntp_tile_outline branch March 25, 2021 11:58
@simonhong simonhong added this to the 1.24.x - Nightly milestone Mar 25, 2021
brave-builds pushed a commit that referenced this pull request Mar 25, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Site-icon's focus ring is too tall, and is clipped beneath the site icon below it
2 participants