Skip to content
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

refactor ntp to use brave-ui #1296

Closed
wants to merge 0 commits into from
Closed

refactor ntp to use brave-ui #1296

wants to merge 0 commits into from

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Jan 11, 2019

screen shot 2019-01-11 at 7 11 00 pm

Changes in this pull-request:

  • Replace all legacy LESS files for NTP
  • Make NTP responsive
  • Simplify logic for background images
  • Remove leftover files

Test Plan 1 - change default new tab and private new tab structure

  • New tab (default) should render;
  • Private tab (default) should render;
  • Private tab (default) shoud toggle between default search engine/ddg;
  • Tor window should render.

Test Plan 2 - refactor new tab page to use brave-ui

close brave/brave-browser#633

  • All interactions in new tab top sites should work

    • Tile links to its website;
    • Pinning/unpinning;
    • Bookmarking/un-bookmarking;
    • Removing
      • All actions in the removal notification should work
  • Background should change upon browser refresh

  • Background should load even without internet connection

  • Links in footer should go to their own links

Test Plan 3 - remove leftover files from ntp

  • Ensure everything builds -- should be catched by CI

Test Plan 4 - adapt favicon size to 64px to match tile icon size

This is a tentative to make favicons look better without much success, but since favicon image wrapper has 64px I thought to be a good idea to have the same size fetched by Brave

@petemill
Copy link
Member

@cezaraugusto nice refactoring - is this ready for review?

@petemill
Copy link
Member

@cezaraugusto ...or does it depend on brave/brave-ui#344?

@cezaraugusto
Copy link
Contributor Author

hey @petemill yes ready now. re brave/brave-ui#344 it has some breaking changes that would be better landing once this PR is reviewed. here I'm using the sha from that PR so not required to have it merged to check out this branch

@cezaraugusto
Copy link
Contributor Author

after a push force github auto closed this pr and can't let me re-open. I'll create a new one

@cezaraugusto
Copy link
Contributor Author

new PR #1339

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

newtab should use brave-ui
2 participants