[Stats Refresh] StatsViewModels: Replace default search engine icon - #11674
Merged
Conversation
Contributor
|
Hey @frosty . The only issue is the image is a bit too big.
The complication is that, before this, all Referrer icons were downloaded, and thus the size is tied to this bit of logic, in |
45 tasks
ScoutHarris
approved these changes
May 20, 2019
ScoutHarris
left a comment
Contributor
There was a problem hiding this comment.
I'll go ahead and approve this. I'll address the image size issue separately.
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refs #11080.
I took an attempt at replacing the search engine icon with a Gridicon as referenced in the issue above.
I figured the view model would be the best place for it, as that's all about transforming the data into the correct format to view. I also opted to switch on the existing icon URL filename instead of the referrer title, as I thought that would change for different locales.
I don't love that it's duplicated in two places, although the rest of the method is already duplicated. If we were to use this in more places, then maybe extracting it somewhere else would make sense. But if you have any suggestions for improvements, or you'd tackle this a different way, let me know!
Before
After
To test: