search

WordPress / browsehappy

101 stars 36 forks source link

Add Brave browser #64

Closed renintw closed 1 year ago

renintw commented 1 year ago

Trac ticket: https://meta.trac.wordpress.org/ticket/6845

Screencast

https://user-images.githubusercontent.com/18050944/225744588-d844c35f-1b16-40c6-b459-dba80ba3b869.mp4

coffee2code commented 1 year ago

Hi @renintw! Thanks for the PR. Since it looks like you might've already done it locally, could you include the updated browsehappy-sprite-2x.png and browsehappy-sprite.png images that include the Brave logo? If not and it was only a mockup for the demo video, that's not a problem and I can update them myself.

renintw commented 1 year ago

Hi @coffee2code! I've updated the sprites. Since I spent some time figuring out how others edit sprites, and I couldn't find out any guide or PRs explaining it so I just calculated the expected new logo width through the original sprites and used the sketch to add it, I'm wondering if this is the same/right way you edited the sprites?

Besides, is the current order acceptable, or I should loop other stakeholders in, like the meta-design?

coffee2code commented 1 year ago

@renintw: That's a perfectly fine way to update the sprite. There isn't much in the way of docs since new logos are rarely added or updated, but documentation is still warranted. Generally, I obtain the 256x256 version of the logo (for use in the 2x sprite; 128x128 for the 1x sprite) from this repo, then edit the existing sprite to replace an existing logo or extend the canvas to add a new logo. Which you've done. (However, that repo's version of the Brave logo appears a bit more orange-ish and less red to me than the actual logo and the logo you've used.)

Anyhow, the PR looks great, though there needs to be one more CSS tweak. In style.css on line 693 of your changed version (it's the very last CSS rule in the file), the background size needs adjusting:

- background-size: 750px 170px;
+ background-size: 875px 170px;

Happy to merge and deploy after that.

Also, the order after your PR is acceptable as-is and shouldn't require looping anyone else in. The order can be changed later if eventually necessary.

renintw commented 1 year ago

@coffee2code thanks for the clarification. I think if anyone else wants to try adding a new browser logo in the future, they can at least refer to the comment here 🙂 I've adjusted the CSS as mentioned. Could you please do the merging and deploying for me? Thanks a bunch!