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

Removed link's underline from settings UI #15585

Merged
merged 1 commit into from
Oct 24, 2022

Conversation

simonhong
Copy link
Member

@simonhong simonhong commented Oct 21, 2022

fix brave/brave-browser#26091

Screen Shot 2022-10-21 at 11 30 58 AM

Below upstream's change enabled underline.
https://chromium-review.googlesource.com/c/chromium/src/+/3855278

WebUI: Decorate links with underline

This is part 1 of decorating all links and all buttons that look like links with the underline to help users better differentiate between normal text and actionable links. This CL updates links on Settings, Extensions, Management, Downloads, and Welcome, as well as updating a couple of global WebUI styles.

Resolves

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
  • 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, 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:

See the linked issue

@simonhong simonhong self-assigned this Oct 21, 2022
@simonhong simonhong requested a review from petemill October 21, 2022 01:10
@simonhong simonhong force-pushed the remove_link_underline_from_settings_ui branch from a027a22 to f5b7019 Compare October 21, 2022 02:21
@simonhong simonhong marked this pull request as ready for review October 21, 2022 02:22
@simonhong simonhong requested a review from a team as a code owner October 21, 2022 02:22
@simonhong simonhong requested a review from emerick October 21, 2022 02:22
@simonhong simonhong force-pushed the remove_link_underline_from_settings_ui branch from f5b7019 to 6c01f54 Compare October 21, 2022 02:28
Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

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

LGTM

fix brave/brave-browser#26091

Below upstream's change enabled underline.
https://chromium-review.googlesource.com/c/chromium/src/+/3855278
WebUI: Decorate links with underline

This is part 1 of decorating all links and all buttons that look like
links with the underline to help users better differentiate between
normal text and actionable links. This CL updates links on Settings,
Extensions, Management, Downloads, and Welcome, as well as updating
a couple of global WebUI styles.
@simonhong simonhong force-pushed the remove_link_underline_from_settings_ui branch from 6c01f54 to 1a9cb91 Compare October 21, 2022 13:48
@simonhong simonhong requested a review from goodov October 21, 2022 13:54
@simonhong simonhong merged commit feaae68 into master Oct 24, 2022
@simonhong simonhong deleted the remove_link_underline_from_settings_ui branch October 24, 2022 05:52
@github-actions github-actions bot added this to the 1.47.x - Nightly milestone Oct 24, 2022
brave-builds pushed a commit that referenced this pull request Oct 24, 2022
brave-builds pushed a commit that referenced this pull request Oct 24, 2022
@petemill
Copy link
Member

Thanks for the fix. I think it's better if we put the css file in brave/browser/resources/settings and phase out page_specific, which was more of a convenience place to place overriding resources for chromium WebUIs where we haven't yet setup our places to inject modifications.

@kjozwiak
Copy link
Member

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.47.14 Chromium: 107.0.5304.62 (Official Build) nightly (64-bit)
-- | --
Revision | 1eec40d3a5764881c92085aaee66d25075c159aa-refs/branch-heads/5304@{#942}
OS | Windows 11 Version 22H2 (Build 22621.675)

Went through the STR/Cases outlined via brave/brave-browser#26091 (comment) and ensured that links weren't being underlined via brave://settings as per the following:

Example Example Example
underlinedFixed1 underlinedFixed2 underlinedFixed3

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.

CR 107 - Links in brave://settings have been underlined and which is inherited from chrome
5 participants