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.

Sign up for GitHub

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

Jump to bottom

[cr120 follow-up] Hide Settings->Appearance hover card toggle. #21166

Merged
merged 1 commit into from
Dec 1, 2023
Merged

[cr120 follow-up] Hide Settings->Appearance hover card toggle. #21166

merged 1 commit into from
Dec 1, 2023

Conversation

mkarolin
Copy link
Collaborator

Resolves brave/brave-browser#34608

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

@mkarolin mkarolin added CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS labels Nov 30, 2023
@mkarolin mkarolin self-assigned this Nov 30, 2023
@github-actions github-actions bot added the CI/storybook-url Deploy storybook and provide a unique URL for each build label Nov 30, 2023
@mkarolin mkarolin force-pushed the maxk-hide-hover-card-image-toggle branch from 820387b to 3e0e2a1 Compare November 30, 2023 22:22
@github-actions GitHub Actions
Copy link
Contributor

[puLL-Merge] - brave/brave-core@21166

This patch is a commit to a version control system (most likely Git). The commit has been made by a developer named 'mkarolin' with the email 'max@brave.com'. The patch is dated November 30, 2023, and the subject line of the commit message is "[PATCH] [cr120 follow-up] Hide Settings->Appearance hover card toggle."

The change is made to the Brave browser, an open-source web browser based on the Chromium web browser. Specifically, the file being modified is part of the browser's settings overrides for the appearance page (appearance_page.ts).

Let's break down the changes made in the patch:

  • The file path of the file being modified is browser/resources/settings/brave_overrides/appearance_page.ts. This suggests that the change is being made to an override file that customizes the appearance settings page in the Brave browser settings.

  • The current state of the file being modified (before applying the patch) has a Git index of 862e0bc9c5c54.

  • The new state of the file (after applying the patch) will have a Git index of 304b42fbbfa37.

  • The patch adds 12 lines of code to the file. This can be seen in the line count diff notation: 12 ++++++++++++.

The actual code being added starts with a comment explaining the purpose of the change:

  • The patch removes the "show images on tab hover toggle" from the settings. The reason is that there is already a setting for this under the Tabs settings. The toggle in question becomes redundant, and thus it's being removed from the Appearance settings page.

The additions include a check for an HTML template element with specific identifiers – is=dom-if and if="[[showHoverCardImagesOption_]]". If the element (hoverCardImagesTemplate) is not found, an error message is displayed in the console to indicate the hover card images template could not be located. If the element is found, it is removed from the DOM, effectively hiding the setting from the user's view.

In a broader context, this patch likely contributes to maintaining a clean and non-redundant user interface within the Brave browser's settings by eliminating a setting toggle that is unnecessary because its function is duplicated elsewhere in the settings.

@brave-builds
Copy link
Collaborator

A Storybook has been deployed to preview UI for the latest push

simonhong
simonhong approved these changes Nov 30, 2023
View reviewed changes
Copy link
Member

@simonhong simonhong left a comment

Choose a reason for hiding this comment

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

++

@mkarolin mkarolin merged commit 8f8581f into master Dec 1, 2023
@mkarolin mkarolin deleted the maxk-hide-hover-card-image-toggle branch December 1, 2023 05:06
@mkarolin mkarolin added this to the 1.63.x - Nightly milestone Dec 1, 2023
mkarolin added a commit that referenced this pull request Dec 1, 2023
@mkarolin
Merge pull request #21166 from brave/maxk-hide-hover-card-image-toggle
30a99a1 
[cr120 follow-up] Hide Settings->Appearance hover card toggle.
@mkarolin mkarolin mentioned this pull request Dec 1, 2023
Merged
25 tasks
@brave-builds brave-builds mentioned this pull request Dec 1, 2023
Merged
7 tasks
brave-builds added a commit that referenced this pull request Dec 1, 2023
@brave-builds
Uplift of #21166 (squashed) to beta
95035c5
@mkarolin mkarolin mentioned this pull request Dec 1, 2023
Merged
25 tasks
@kjozwiak
Copy link
Member

kjozwiak commented Dec 1, 2023

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

Brave | 1.63.6 Chromium: 120.0.6099.56 (Official Build) nightly (64-bit)
-- | --
Revision | b824c6a6d09d59e82d9f30f999ef9afb172cbc0b
OS | Windows 11 Version 22H2 (Build 22621.2715)

Using the STR/information via brave/brave-browser#34608 (comment), ensured that the redundant Show images on tab hover preview cards toggle was removed from brave://settings/appearance as per the following:

  • ensured that Tab hover mode was still visible under Tabs via brave://settings/appearance
  • ensured that Card was set as the default on new installs/new profile
  • ensured that Card with preview & Tooltip were working as expected
Example Example
image image

kjozwiak pushed a commit that referenced this pull request Dec 1, 2023
@brave-builds
[cr120 follow-up] Hide Settings->Appearance hover card toggle. (uplif…
c78ffbc 
…t to 1.62.x) (#21179)

Uplift of #21166 (squashed) to beta
@kjozwiak kjozwiak mentioned this pull request Dec 1, 2023
Closed
kjozwiak pushed a commit that referenced this pull request Dec 1, 2023
@mkarolin @cdesouza-chromium
@emerick @samartnik @AlexeyBarabash
Upgrade from Chromium 119 to Chromium 120 (uplift to 1.61.x). (#21135)
587ce3e 
* Merge pull request #20434 from brave/cr120

Upgrade from Chromium 119 to Chromium 120.

* Updated chrome-VERSION.patch from Chromium 119.0.6045.163 to Chromium 120.0.6099.35.

* Additional assert_ts.js -> assert.js for 1.61.x.

Chromium change:

https://chromium.googlesource.com/chromium/src/+/f7a2ea671005530e7a891bba411a34bfd1b3b4cd

commit f7a2ea671005530e7a891bba411a34bfd1b3b4cd
Author: dpapad <dpapad@chromium.org>
Date:   Mon Oct 16 21:13:04 2023 +0000

    WebUI: Rename assert_ts.ts to assert.ts, part 7 (last)

    In this part updating one remaining reference in ui/file_manager/ and
    removing assert_ts.ts.

    assert_ts.ts used the "_ts" suffix to differentiate from the previously
    existing assert.js file. The latter no longer exists and the "_ts"
    suffix can be dropped.

    Fixed: 1291526

* Remove `SidePanelCoordinator` `final` keyword substitution (#20765)

`SidePanelCoordinator` is tagged upstream as `final`, and as a
workaround to be able to derive from it, an override had been introduced
at some point to replace the keyword `final` with naught. This type of
redefinition is unreliable, as it is UB, and it also becomes highly
viral.

This change replaces the substitution with a patch. The substitution was
removing the `final` keyword from the signature of other functions in
different headers, and leaving it with no token to indicate the function
is an override, causing a build failure.

* `Label::CalculatePreferredSize()` marked as `final` (#20767)

This function is now marked as `final`, however there is another entry
in the overload set that can be used in its place for the override in
`BraveVPNStatusLabel`.

Chromium change:
https://chromium.googlesource.com/chromium/src/+/bf2f84a8c4f27e399e05bf4730a29152f6a8b0b6

commit bf2f84a8c4f27e399e05bf4730a29152f6a8b0b6
Author: weidongliu <liuwd8@gmail.com>
Date:   Mon Oct 30 15:59:03 2023 +0000

    views: Mark CalculatePreferredSize() of views::Label as final

    Views::Label now has two functions to calculate the preferred size. This
    can cause some unexpected errors. CalculatePreferredSize() is now
    expected to be deprecated. Therefore, mark it as final to prevent
    overrided by subclasses.

    Bug: 1346889

* Revert "Disable kPowerBookmarksSidePanel feature flag"

This reverts commit 237341e.

* Java files formatting.

* Fix `BraveBrowsingDataRemoverDelegate` substitution (#21034)

This override has been missing a matching header since it was
introduced, and this has been causing linking to fail in every upstream
occurrence of `ChromeBrowsingDataRemoverDelegateFactory::GetForProfile`
as the return type in the header was not being replaced accordingly.

This changes adds the changes to the header as well, and provides an
inclusion `BraveBrowsingDataRemoverDelegate` in the shadow file, to
allow the compiler to understand that
`BraveBrowsingDataRemoverDelegate*` is pefectly convertible to
`ChromeBrowsingDataRemoverDelegate`.

* [Android] Move JNI classes to jni_zero namespace

* Disabled failing upstream browser tests.

* Merge pull request #21152 from brave/maxk-fix-page-info-cookies-separator

[cr120 follow-up] Removes an extra separator in cookies page info view.

* Merge pull request #21153 from brave/maxk-redirect-on-device-data-to-cookies

[cr120 follow-up] Redirect On-device data link to cookies page.

* Merge pull request #21166 from brave/maxk-hide-hover-card-image-toggle

[cr120 follow-up] Hide Settings->Appearance hover card toggle.

* Fixed crash accessing 'send to devices' in share menu via Sync (#21168)

Fixes brave/brave-browser#34636

Related Chromium commit:
https://source.chromium.org/chromium/chromium/src/+/84ed1f62ecd64bbfc3d2c4775a6e80a6b0e9defb

Clean up SendTabToSelfSigninPromo flag

Enabled by default in M119 (https://crrev.com/c/4905024), can be
cleaned up in M120.

Fixed: 1479693, 1295204
Change-Id: Ic778e9c886a5d9391fb5da7510d166952481fe21
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4905343
Reviewed-by: Boris Sazonov <bsazonov@chromium.org>
Reviewed-by: Wenyu Fu <wenyufu@chromium.org>
Code-Coverage: findit-for-me@appspot.gserviceaccount.com <findit-for-me@appspot.gserviceaccount.com>
Commit-Queue: Victor Vianna <victorvianna@google.com>
Cr-Commit-Position: refs/heads/main@{#1204655}

* Merge pull request #21169 from brave/120.0.6099.56_master

Upgrade from Chromium 120.0.6099.35 to Chromium 120.0.6099.56.

* Updated chrome-VERSION.patch from Chromium 120.0.6099.35 to Chromium 120.0.6099.56.

---------

Co-authored-by: cdesouza-chromium <cdesouza@brave.com>
Co-authored-by: Emerick Rogul <erogul@brave.com>
Co-authored-by: Artem Samoilenko <artem@brave.com>
Co-authored-by: AlexeyBarabash <AlexeyBarabash@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@simonhong simonhong simonhong approved these changes

@fallaciousreasoning fallaciousreasoning Awaiting requested review from fallaciousreasoning

Assignees

@mkarolin mkarolin

Labels
CI/skip-android Do not run CI builds for Android CI/skip-ios Do not run CI builds for iOS CI/storybook-url Deploy storybook and provide a unique URL for each build
Projects
None yet
Milestone
1.63.x - Release
Development

Successfully merging this pull request may close these issues.

CR 120 - Redundant settings Show images on tab hover preview cards is inherited from chrome
4 participants
@mkarolin @brave-builds @kjozwiak @simonhong