Switch to using more official browser brand icons

[hiramzamorano]

• Closes Switch to using more official browser brand icons #5921

User facing changelog

We replaced the browser icons in the Test Runner with more official browser brand icons.

How has the user experience changed?

Before

After

PR Tasks

• Have tests been added/updated?

[cypress-bot]

Thanks for the contribution! Below are some guidelines Cypress uses when doing PR reviews.

• Please write [WIP] in the title of your Pull Request if your PR is not ready for review - someone will review your PR as soon as the [WIP] is removed.
• Please familiarize yourself with the PR Review Checklist and feel free to make updates on your PR based on these guidelines.

PR Review Checklist

If any of the following requirements can't be met, leave a comment in the review selecting 'Request changes', otherwise 'Approve'.

User Experience

• The feature/bugfix is self-documenting from within the product.
• The change provides the end user with a way to fix their problem (no dead ends).

Functionality

• The code works and performs its intended function with the correct logic.
• Performance has been factored in (for example, the code cleans up after itself to not cause memory leaks).
• The code guards against edge cases and invalid input and has tests to cover it.

Maintainability

• The code is readable (too many nested 'if's are a bad sign).
• Names used for variables, methods, etc, clearly describe their function.
• The code is easy to understood and there are relevant comments explaining.
• New algorithms are documented in the code with link(s) to external docs (flowcharts, w3c, chrome, firefox).
• There are comments containing link(s) to the addressed issue (in tests and code).

Quality

• The change does not reimplement code.
• There's not a module from the ecosystem that should be used instead.
• There is no redundant or duplicate code.
• There are no irrelevant comments left in the code.
• Tests are testing the code’s intended functionality in the best way possible.

Internal

• The original issue has been tagged with a release in ZenHub.

[claassistantio]

All committers have signed the CLA.

[jennifer-shehane]

@hiramzamorano Thanks for the contribution! Could you please sign our CLA? #5991 (comment)

[jennifer-shehane]

• All of the tests verifying the icons are correct have been removed. We need tests that the correct image displays, not just any image. Removing the test for the custom browser, I can already manually see that the image is incorrect. This could be as simple as adding a class to each img, so testing that when Chrome is selected that the img.chrome is shown.
• We need a custom 'globe' icon for browsers that are not defined. Users can pull in custom browsers (like Brave) and we don't want them to show up as Electron.

[hiramzamorano]

Hi @jennifer-shehane, thanks for the feedback, i'm going to fix those issues :)

[jennifer-shehane]

The icons are not all the same width and height, which makes them not line up properly vertically in the dropdown and also the defaultBrowser icons doesn't line up vertically in the selection when selected.

without guidelines

with guidelines to show horizontal and vertical places that are not aligned.

[jennifer-shehane]

I made a few commits to add back a missing test and fix the aligning of the icon in the selection.

My final comment is about the default generic icon. It doesn't look great to be honest. Is there a nicer looking 'world/globe' / generic browser icon you can use? It also shows up much larger than the other icons for some reason even thought the width and height are 16.

[hiramzamorano]

You're right @jennifer-shehane , we can improve the default generic icon to make it look better.
I can suggest you these options.

1)

2)

3)

4)

5)

6)

What do you think?

[jennifer-shehane]

@hiramzamorano I prefer #5. Thanks for providing the options. 👍

[jennifer-shehane]

The default browser icon still does not line up visually with the other icons. It appears larger and juts out to the left of all the other icons.

This will be good to merge in after this is fixed.

[jennifer-shehane]

@hiramzamorano I am not seeing any actual change in the display/alignment after the latest change.

You can see this view by in the packages/desktop-gui directory running npm run watch then opening a new terminal and running npm run cypress:open. In here run the project_nav_spec and run the custom browser available set of tests. Select 'Chrome' which will put the Foo browser in the dropdown.

[hiramzamorano]

It seems that the previous commit had no effect on the icon, that's weird because I was testing it with the steps you tell me.
The solution is to center the svg image in its own container, I made some small changes hoping that now if they are reflected.

This should be the end result.

[brian-mann]

The electron logo should not be electron - it should be dark Chrome. Our users don't care what the version of Electron is, they care about the version of Chromium that electron ships with.

On second thought - it may make more sense to use the Electron icon with the displayName as Electron (Chromium) 76

[jennifer-shehane]

@hiramzamorano The team decided to revert this commit per these comments. #6141 (comment)

You're welcome to open a new PR. I'm sorry for the outcome of this. No one expressed their differing opinions from the team until after it was merged unfortunately.