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

New Tab Page: Move from brave-ui to brave-core #2857

Merged
merged 43 commits into from
Jul 8, 2019
Merged

Conversation

petemill
Copy link
Member

@petemill petemill commented Jul 3, 2019

Fix brave/brave-browser#2518 (add storybook to brave-core)
Partially address brave/brave-browser#2335 (umbrella)
Fix brave/brave-browser#5178

This moves all New Tab Page specific UI and Storybook code from brave-ui. It includes all the history from brave-ui where the NTP files were modified.

Sets up at brave-core specific Storybook at npm run storybook

The steps I performed

Will be useful for moving the other brave-ui features to brave-core

Reduce brave-ui to necessary folders:

# clone brave-ui to a temp location
git clone -s brave-ui brave-ui-temp-move-ntp
cd brave-ui-temp-move-ntp

# remove everything else
git checkout -b only-ntp
git filter-branch --index-filter 'git rm --cached -qr --ignore-unmatch -- . && git reset -q $GIT_COMMIT -- src/features/newTab stories/features/newTab' --prune-empty -- --all

# then clean working dir
git reset --hard

# move to where we want them in brave-core

# -EITHER- manual:

mkdir -p components/brave_new_tab_ui/components
git mv src/features/newTab/** components/brave_new_tab_ui/components

mkdir -p components/brave_new_tab_ui/stories
git mv stories/features/newTab/** components/brave_new_tab_ui/stories

git commit -m "New Tab Page WebUI: Move brave-ui paths to brave-core paths

# -OR- automatic, for simple cases where everything has always been in the same place:

git filter-branch --tree-filter '(test -d src/features/newTab && mkdir -p components/brave_new_tab_ui/components && mv src/features/newTab/* components/brave_new_tab_ui/components && mkdir -p components/brave_new_tab_ui/stories && mv stories/features/newTab/* components/brave_new_tab_ui/stories) || echo "Did not exist"' HEAD

# Prepend string to commit message
git filter-branch -f --msg-filter 'printf "New Tab Page: Imported from brave-ui - " && cat' [first commit ref]..HEAD
  1. Move brave-core files
cd brave-core
git checkout -b brave-ui-ntp
git mv components/brave_new_tab_ui/components components/brave_new_tab_ui/containers
# First, change any references from components/ to containers/
...
# Check everything still builds and works, then...
git commit -m "NTP WebUI: Reclassify existing components as containers"
  1. Pull reduced brave-ui history to brave-core repo
# In brave-core repo path (brave-browser/src/brave)
git remote add tmp-brave-ui-ntp ../../../brave-ui-temp-justntp
git pull tmp-brave-ui-ntp temp-justntp --allow-unrelated-histories
git remote rm tmp-brave-ui-ntp
  1. Find any missing npm dependencies:
    Do a project search...
    Find (regex): from '[^\.].*'
    In: brave/components/brave_new_tab_ui/{components,stories}/**/*.{ts,tsx}
    Then npm install --save any that aren't already in brave-core
  2. Find TS import paths which need to change
    Do a project find / replace
    Find (regex): '(\.\.\/)+(theme|components)'
    In: brave/components/brave_new_tab_ui/components/**/*.{ts,tsx}
    Replace with: 'brave-ui/$2
  3. Find storybook TS import paths which need to change
    Do a project find / replace
    Find (regex): '(\.\.\/)+src([^']*)'
    In: brave/components/brave_new_tab_ui/stories/**/*.{ts,tsx}
    Replace with: 'brave-ui$2'
  4. Change brave-ui feature refs to components/ refs
    Do a project find
    Find (regex): brave-ui(/src)?/features/newTab
    In: brave/components/brave_new_tab_ui/{components,containers,stories}/*/*.{ts,tsx}
    Replace with: ../../components
    Then repeat adding an extra /* and an extra ../ for as many folder depths until we think we have them all :-)
  5. Test TS / Webpack build
npm run web-ui -- --mode=production brave_new_tab_ui=/Users/petemill/Development/Brave/brave-browser/src/brave/components/brave_new_tab_ui/brave_new_tab.tsx
  1. Test storybook build
npm run storybook

Fix any errors, in NTP case:
* Removed muli.css and poppins.css import (now inserted automatically)
* Fixed inconsistent assets/img path and file naming compared to brave-core
(In NTP case: assets/img paths)

  1. Rename stories
    Do a project find / replace
    Find (regex): 'Feature Components/New Tab'
    In: brave/components/brave_new_tab_ui/stories/**/*.{ts,tsx}
    Replace with: 'New Tab'

Test Plan:

NTP works

  • Open NTP
  • Check renders and no console errors

NTP is coming from brave-core not brave-ui

  • Modify components/brave_new_tab_ui/components/[something]
  • Build brave browser again
  • Check modification is visible on NTP

Storybook works

  • npm run storybook
  • Check NTP is visible in Storybook
  • Modify components/brave_new_tab_ui/components/[something]
  • Check modification is visible on Storybook with hot-reloading

Submitter Checklist:

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

@petemill
Copy link
Member Author

petemill commented Jul 3, 2019

🤦‍♀ I need to rebase this to include #2762

NejcZdovc and others added 24 commits July 5, 2019 16:18
- generally updated terminology to more consistently refer to windows instead of tabs
- updated Tor communication on the private window to instruct users how to access a tor protected window through the file menu
- updated to communication to omit speaking about DDG as if it is enabled by default (because it isn't right?)
- new copy
- updated some padding on modals
- small opacity and styling tweaks
- typography styling updates
- temporarily removed DDG learn more links until appropriate knowledgebase articles can be written
- followed the breakpoint pattern for qwant as the content was overlapping eachother at 490px
- added a Qwant with Tor option
- updated copy for Qwant Tor
-
wrongly removed after 15582bb879cc7fef3bfce674841e6d4b6a3f013a
- styling related to NTP updates still linked to Brave UI
- removed the Styled Period designation from the clock component
- uploaded 4 new background images (only one is registering but the others are there for when randomization is implemented)
- uploaded correct icons as components and put them in the right places
- general padding and margin updates
- general typography and coloring updates
- some responsive styling and breakpoint updates
- moved tiles to left instead of right of screen
imptrx and others added 8 commits July 5, 2019 16:18
…ggle-background-img"

This reverts commit dbc5cda1cc182fca4813b129176a629c37d8d679, reversing
changes made to 0139aa32246af6d4e4ec0daea79945966dd79999.
Revert "Merge pull request #490 from brave/revert-485"

This reverts commit 82b3e981040b2ba89743108e6701f5fa24ce3b52, reversing
changes made to b0ec08cd4ed2a20401d058062f1c238fb9a67813.
@petemill petemill force-pushed the brave-ui-ntp branch 2 times, most recently from 93094ce to 2553ed7 Compare July 6, 2019 00:01
@petemill
Copy link
Member Author

petemill commented Jul 6, 2019

Rebase complete, review appreciated

petemill and others added 4 commits July 8, 2019 10:58
Prerequisite to New Tab Page storybook, which uses .webp background images
Fix component and storybook import paths
Convert existing brave-core containers to use component import paths import paths, instead of brave-ui
@petemill
Copy link
Member Author

petemill commented Jul 8, 2019

Jenkins and Travis now both passed. Test plan is included and Issues created / updated. Ready for review ASAP please.

Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtmplate

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