-
Notifications
You must be signed in to change notification settings - Fork 889
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.
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
NTP Refinement #1091
NTP Refinement #1091
Conversation
cacfcc5
to
036e14c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Initial code review pass looks great - thanks for putting all that painstaking work in to tweak everything @rossmoody!
As discussed we'll let the code format changes to non-changed code slide since it's hard to undo, but now we know for future that it makes it more difficult to review when unrelated blocks of code changes.
I'll need to get a build going to take a 2nd pass.
I left some comments on cleaning up dead code, and more usefully seeing if we can remove font-awesome by putting in 2 svg files (ours or theirs). I can help on the hopefully minor css changes that that may necessitate. If it's a pain, we can certainly do that as a follow-up.
Also I wanted to ask - are the licenses for these 16 new images permissive for us to bundle and use like this? Do we need to check with someone that knows these things better than I do?
@petemill Changes updated including the icon swaps from fontawesome. Omitted dead code and made a few additional updates to facilitate the new icons. We are good to go on the imagery rights front. These are pulled from Unsplash, Pexel, and SpaceX Flickr which all grant commercial licenses(spacex is public domain). 3 other images are from a photographer (Will Christiansen) that reached out through business dev and granted us license to use with credit (which we do anyway). |
1b0c195
to
f2b5fc8
Compare
-updated images -refine new tab responsiveness
-change from stark white to dark grey until further investigation on putting images on body
2f2bb4d
to
8f2bd2b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ lots of bugs fixed! great job on this one, Ross. NTP is looking 👌
{ "Phoyoserge_TheSeantParis.jpg", IDR_BRAVE_NEW_TAB_BACKGROUND18 }, | ||
{ "Phoyoserge_VeniseSunset.jpg", IDR_BRAVE_NEW_TAB_BACKGROUND19 }, | ||
{ "Phoyoserge_Yosemite.jpg", IDR_BRAVE_NEW_TAB_BACKGROUND20 }, | ||
{ "ntp-1.webp", IDR_BRAVE_NEW_TAB_BACKGROUND1 }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we are so cool with webp
feedback addressed by the author
master b10ed21 |
fixes brave/brave-browser#2504
fixes brave/brave-browser#1779
fixes brave/brave-browser#494
fixes brave/brave-browser#1034
fixes brave/brave-browser#341
fixes brave/brave-browser#1593
fixes brave/brave-browser#1876
This started as a PR to replace NTP background images only but I noticed lots of small polish items that needed addressed. A mockup of all 16 new images is below as well as a before and after of changes + updated responsive behavior.
Most of these changes are still not optimal but I imagine the NTP is slated for a larger refactor in correlation with Brave UI workflow when time allows. This is just to fix things that feel outright wrong and distance the image selection from Muon.
Updates
JPG
towebp
to save an additional 25% percent file size.title
so it was omitted from the photo credit. IME the titles lead to typos and mistakes that need constantly maintained and really elongate the time to upload new images so we can bring it back but it makes changing images much more time consuming.