Bug 1897319 - Stores icons for light/dark theme for Yelp suggestion #6248

dadaa · 2024-05-16T23:47:04Z

Bugzilla: https://bugzilla.mozilla.org/show_bug.cgi?id=1897319
Jira: https://mozilla-hub.atlassian.net/browse/SNG-1515

We need to update the UI for Yelp suggestion.
One of them is to chnage the logo. But it has to change by the light/dark theme. Thus, make it to store both icons.

This PR did two things.

Stores both icons and change the related codes.
Introduce icon struct as same as PR Bug 1884816 - Expose icon information for suggestion as struct #6162 (A warning that said paramters are too complex in the first commit, it was a good chance :))

Pull Request checklist

Breaking changes: This PR follows our breaking change policy
- This PR follows the breaking change policy:
  - This PR has no breaking API changes, or
  - There are corresponding PRs for our consumer applications that resolve the breaking changes and have been approved
Quality: This PR builds and tests run cleanly
- Note:
  - For changes that need extra cross-platform testing, consider adding [ci full] to the PR title.
  - If this pull request includes a breaking change, consider cutting a new release after merging.
Tests: This PR includes thorough tests or an explanation of why it does not
Changelog: This PR includes a changelog entry in CHANGELOG.md or an explanation of why it does not need one
- Any breaking changes to Swift or Kotlin binding APIs are noted explicitly
Dependencies: This PR follows our dependency management guidelines
- Any new dependencies are accompanied by a summary of the due diligence applied in selecting them.

Branch builds: add [firefox-android: branch-name] to the PR title.

codecov-commenter · 2024-05-16T23:55:51Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 50.85%. Comparing base (8bd7ba4) to head (1907a14).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6248   +/-   ##
=======================================
  Coverage   50.85%   50.85%           
=======================================
  Files         112      112           
  Lines       11811    11811           
=======================================
  Hits         6006     6006           
  Misses       5805     5805

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

0c0w3

Thanks Daisuke, r+ with comments -- especially, the schema version needs to be bumped and a migration step added.

0c0w3 · 2024-05-17T00:08:53Z

components/suggest/src/schema.rs

You'll need to bump the schema VERSION const and add a migration step to upgrade_from(). See the comment above VERSION.

0c0w3 · 2024-05-17T00:25:43Z

components/suggest/src/store.rs

Thanks for updating all these tests, I know it's pain.

It looks like we now have tests for Yelp suggestions with both light and dark icons, but no tests for only one or the other. Also we may want to keep including icon in the RS data in order to support older Firefox versions for some time, and I want to make sure that doesn't break newer versions.

So could you add cases for the following please, in whatever way is easiest? In all cases, ingest and fetch should complete successfully.

Only iconLightTheme is present in the RS data

Only iconDarkTheme is present

icon, iconLightTheme, and iconDarkTheme are all present (icon should be ignored and ingest and fetch should still be successful)

Good idea! Thanks!

0c0w3 · 2024-05-17T00:33:56Z

components/suggest/src/schema.rs

+    icon_light_theme_id TEXT NOT NULL,
+    icon_dark_theme_id TEXT NOT NULL,


Storing both light and dark icons in the same row is OK with me, and also I just wanted to mention another way to do it would be to have one icon per row plus an "icon type" column. If we need to support any more types of icons in the future -- and I don't know why we would! -- I think we should switch to that approach.

Yeah, if we do not know how many icons we should handle, then yeah, should introduce the mechanism.

0c0w3

Thanks, it looks like there are merge conflicts in lib.rs and store.rs, plus some failing tests. Could you fix those and make sure cargo test -p suggest passes locally for you?

0c0w3 · 2024-05-20T21:42:52Z

components/suggest/src/schema.rs

@@ -193,6 +195,11 @@ CREATE TABLE IF NOT EXISTS dismissed_suggestions (
                )?;
                Ok(())
            }
+            19 => {
+                // Need to update new icons for Yelp especially in new schema.
+                let _ = clear_database(tx);


Since only the Yelp table changed, can we do something like this instead?

If yelp_custom_details exists, get the current record from yelp_custom_details and then DROP TABLE yelp_custom_details

CREATE TABLE yelp_custom_details just like in the main schema SQL

If we got the record from the old table in step 1, insert it into the new table, using the old icon_id as the new icon_light_theme_id

Starting with schema version 16, I believe the idea is to avoid clearing the entire DB on upgrade if possible.

Sorry to add more work but It would also be nice if you could add a test for this migration step too.

Okay, thanks!

dadaa · 2024-05-21T06:28:31Z

Hi @0c0w3! As I changed more places than I expected, I'd like to request review from you again. Thanks!

0c0w3 · 2024-05-21T20:29:36Z

Hey @dadaa, Emilio's idea of choosing the appropriate image in the svg itself is really nice, and it means we don't need to modify the schema at all. I'm sorry for not thinking of that, and for all the work you've done in this PR that we might not need.

If you remove the schema change from this PR, it's basically the same as #6162, right?

dadaa · 2024-05-21T20:50:17Z

Hey @dadaa, Emilio's idea of choosing the appropriate image in the svg itself is really nice, and it means we don't need to modify the schema at all. I'm sorry for not thinking of that, and for all the work you've done in this PR that we might not need.

Yeah, indeed. I'm also sorry to you that I took your time due to I could not come up with it.

If you remove the schema change from this PR, it's basically the same as #6162, right?

Yes! So, I will close this PR when the bug is fixed.

dadaa · 2024-05-21T20:59:02Z

Hey @dadaa, Emilio's idea of choosing the appropriate image in the svg itself is really nice, and it means we don't need to modify the schema at all.

Yeah, indeed. I'm also sorry to you that I took your time due to I could not come up with it.

But, I was a little concerned about whether it would be okay to change the official logo file, even though it would look the same.

0c0w3 · 2024-05-21T23:02:50Z

I think it's OK to create a new file that combines both official files as long as we don't modify any strokes or colors. My example file in Phabricator embeded only the <path>s from each file, but I think it's possible to embed the whole <svg> from each file inside a <g> in the combined file. That way, we're really not modifying anything at all, only combining.

dadaa · 2024-05-22T00:12:44Z

We took another approach, let me close this PR.
https://phabricator.services.mozilla.com/D210745

dadaa requested review from 0c0w3 and linabutler May 16, 2024 23:47

0c0w3 approved these changes May 17, 2024

View reviewed changes

dadaa force-pushed the yelp-theme-icon branch from 5fb03e6 to 9bc023d Compare May 17, 2024 14:33

0c0w3 reviewed May 20, 2024

View reviewed changes

Supports icons dependent on theme

a5b133a

dadaa force-pushed the yelp-theme-icon branch from 9bc023d to 7d62724 Compare May 21, 2024 06:24

dadaa requested a review from 0c0w3 May 21, 2024 06:28

Introduce suggestion icon struct

1907a14

dadaa force-pushed the yelp-theme-icon branch from 7d62724 to 1907a14 Compare May 21, 2024 12:44

dadaa closed this May 22, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bug 1897319 - Stores icons for light/dark theme for Yelp suggestion #6248

Bug 1897319 - Stores icons for light/dark theme for Yelp suggestion #6248

dadaa commented May 16, 2024

codecov-commenter commented May 16, 2024 •

edited

Loading

0c0w3 left a comment

0c0w3 May 17, 2024

0c0w3 May 17, 2024

dadaa May 17, 2024

0c0w3 May 17, 2024

dadaa May 17, 2024

0c0w3 left a comment

0c0w3 May 20, 2024

dadaa May 20, 2024

dadaa commented May 21, 2024

0c0w3 commented May 21, 2024

dadaa commented May 21, 2024

dadaa commented May 21, 2024

0c0w3 commented May 21, 2024

dadaa commented May 22, 2024

		icon_light_theme_id TEXT NOT NULL,
		icon_dark_theme_id TEXT NOT NULL,

Bug 1897319 - Stores icons for light/dark theme for Yelp suggestion #6248

Bug 1897319 - Stores icons for light/dark theme for Yelp suggestion #6248

Conversation

dadaa commented May 16, 2024

Pull Request checklist

codecov-commenter commented May 16, 2024 • edited Loading

Codecov Report

0c0w3 left a comment

Choose a reason for hiding this comment

0c0w3 May 17, 2024

Choose a reason for hiding this comment

0c0w3 May 17, 2024

Choose a reason for hiding this comment

dadaa May 17, 2024

Choose a reason for hiding this comment

0c0w3 May 17, 2024

Choose a reason for hiding this comment

dadaa May 17, 2024

Choose a reason for hiding this comment

0c0w3 left a comment

Choose a reason for hiding this comment

0c0w3 May 20, 2024

Choose a reason for hiding this comment

dadaa May 20, 2024

Choose a reason for hiding this comment

dadaa commented May 21, 2024

0c0w3 commented May 21, 2024

dadaa commented May 21, 2024

dadaa commented May 21, 2024

0c0w3 commented May 21, 2024

dadaa commented May 22, 2024

codecov-commenter commented May 16, 2024 •

edited

Loading