Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Refactor navigationBar with Aphrodite - master #9851

Closed
wants to merge 7 commits into from

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Jul 3, 2017

See #9299 for pre-work.

The main things that this PR tries to do are:

  • Refactor styles of the files in navigation folder, except rules with media queries (as there seems to be room for optimization) and for Windows.
  • Create components if the code structure gets complicated, adding BEM Level comments.
    • By adding such comments, it would be easy to understand what the code structure is like.

Closes #9846
Addresses #9283

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Test Plans

Test Plan (obsolete styles)

  1. Search inputbar-wrapper on your repo
  2. Search navbarMenubarBlockContainer
  3. Search your repo for @siteEVColor
  4. Search your repo for @loadTimeColor
  5. Make sure nothing is found

Since automated tests cannot find visual regressions, please conduct these tests carefully from perspectives of clickability, draggability, animation, color, etc. Thanks!

Test Plan (top panel):

  1. Open about:preferences#advanced
  2. Enable Always show the URL bar
  3. Make sure the navigation bar is displayed, aligned horizontally
  4. Make sure the URL bar is displayed
  5. Make sure the buttons on the navigation bar are displayed
  6. Input something on the URL bar
  7. Make sure the URL bar is highlighted
  8. Set focus to input area on URL bar
  9. Make sure the blue outline is displayed
  10. Make sure margin-right:3px is applied
  11. Make sure border-radius:0 is applied

Test Plan (add bookmark icon)

  1. Make sure clicking the star icon bookmarks a page
  2. Make sure the bookmark star icon is filled
  3. Remove the bookmark
  4. Make sure bookmark star icon becomes blank

Test Plan (buttons inside the top panel):

  1. Open about:preferences#advanced
  2. Enable Always show the URL bar
  3. Open https://brave.com
  4. Make sure both bookmark button and add publisher button are rendered
  5. Click the bookmark star button container
  6. Make sure the page is bookmarked
  7. Remove the bookmark
  8. Make sure bookmark star icon becomes blank
  9. Make sure the whole container of the publisher button is clickable (click the edge of the button)

Test Plan (Publisher Toggle):

  1. Disable Brave Payments
  2. Open brave.com
  3. Make sure the empty container does not appear on the right of the URL bar

Test Plan (navigation buttons):

  1. Make sure go back and forward button do not have the background and box-shadow when they are disabled
  2. Click the edge of the reload button
  3. Make sure the button is clicked
  4. Click the edge of the load stop button
  5. Make sure the button is clicked
  6. Show the home button
  7. Click the edge of the button
  8. Make sure your home page is loaded

Test Plan (fullscreen mode)

  1. Enable the fullscreen mode
  2. Make sure the navigation buttons (go back/forward) are aligned to left

Test Plan (elements around the URL bar):

  1. Move your mouse pointer out of the top panel
  2. Make sure .urlbarForm does not have a white background
  3. Open about:preferences#advanced
  4. Disable the wide URL bar
  5. Change window width
  6. Make sure there is margin between the URL bar and the lion icon
  7. Make sure the URL bar appears at the center on the navigation bar

Test plan (loadTime)

  1. Open https://brave.com
  2. Click the loading timer
  3. Make sure the loading timer is displayed
  4. Make sure the loading timer cannot be selected

Test Plan (URL bar un/lock icon)

  1. Enter data:text/html,hi in the URL bar (ee79fc7)
  2. Make sure the triangle is red
  3. Open https://brave.com
  4. Make sure the fa-lock icon is green
  5. Open a new dashboard
  6. Make sure the search icon is grey
  7. Disable Always show the URL bar if not
  8. Open http://http.badssl.com/
  9. Make sure the unlock icon is not transparent in the title mode
  10. Make sure the urlbar icon (fa-un/lock or fa-search) is placed at the center of the icon's container
  11. Input :a to the URL bar
  12. Make sure styles.urlbarIcon is applied to the amazon favicon
  13. Move your mouse pointer out of the top panel
  14. Make sure the .urlbarForm does not have a white backgroundf
  15. Open about:preferenes
  16. Make sure the fa-list icon is moved down 1px on URL bar

Test Plan (NoScript):

  1. Block scripts
  2. Make sure NoScript icon on the URL bar is clickable
  3. Make sure the icon is large enough to be clicked easily
  4. Make sure the un/lock icon, bookmark icon, NoScript icon, and publisher button are aligned horizontally
  5. Make sure the padding between the URL bar edge and noScript button is 3px
  6. Allow scripts
  7. Make sure the padding between the URL bar edge and load timer is 10px

Test Plan (bookmark hanger)

  1. Bookmark a page
  2. Make sure arrowUp appears under the bookmark star icon
  3. Enable Home button
  4. Repeat the step 2

Test plan (for Windows)

  1. On Windows, make sure the specified styles are applied
  2. On Windows, make sure .topLevelStartButtons has padding-left:5px

Test Plan 8 (for macOS):

  1. On macOS, make sure .topLevelStartButtons has padding-left:76px
  2. On macOS, make the browser window to fullscreen
  3. Make sure the left buttons on the navigation bar is moved to the left
  4. Make sure the margin-right on navigator is gone

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

.getAttribute(urlbarIcon, 'class').then((classes) =>
.waitForExist(urlBarIcon)

// TODO: Fix
Copy link
Contributor Author

@luixxiul luixxiul Jul 3, 2017

Choose a reason for hiding this comment

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

@NejcZdovc would you mind fixing the test errors including below? thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

fixed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks!

@luixxiul luixxiul force-pushed the navigationBar-aphrodite branch 4 times, most recently from 7a9562f to b8ea9ee Compare July 4, 2017 04:41
@luixxiul luixxiul requested a review from cezaraugusto July 4, 2017 05:19
@luixxiul luixxiul force-pushed the navigationBar-aphrodite branch 14 times, most recently from f1ab4ef to 836ee4e Compare July 5, 2017 03:24
@cezaraugusto
Copy link
Contributor

@luixxiul can you pls rebase against master latest? I was going to jump in but having problems with spellchecker. I guess two missing parts are the getter thing and 1-5 in https://travis-ci.org/brave/browser-laptop/jobs/255581013#L3262 correct? cc @NejcZdovc

@NejcZdovc
Copy link
Contributor

@cezaraugusto Yes that's correct

Suguru Hirahara and others added 7 commits July 28, 2017 09:26
Addresses #9283

.navbarMenubarBlockContainer (introduced with f2426f1#diff-303f0b6a297506f2cc18bf7b4cb370c5R789, which no longer exists on the master branch.)

.inputbar-wrapper (introduced with 0e57690#diff-02c4b23ad267fe636760179e32fa29ceR141 with no clear reason. Since .inputbar-wrapper has not been used, it can be removed safely.)
Addresses #9283

Also:
- Replace stopButton, homeButton, publisherButton, and braveMenuButton
Addresses #9283

See discussion on #9851 (comment)
@luixxiul
Copy link
Contributor Author

luixxiul commented Jul 28, 2017

@cezaraugusto it seems Aphrodite 1.1.0 has a bug that the cascaded properties are not properly applied, which seems to have been fixed with 1.2.0 (and 1.2.3).

eg. on this case:

background: `url(${bookmarkButtonIcon}) center no-repeat`,
backgroundSize: '14px 14px'

Blocked on #10165.

@luixxiul
Copy link
Contributor Author

luixxiul commented Jul 28, 2017

The upstream bug of Aphrodite seems to have been fixed with Khan/aphrodite@3d4a5bd reported here: Khan/aphrodite#199

@luixxiul luixxiul mentioned this pull request Aug 1, 2017
8 tasks
@luixxiul luixxiul modified the milestones: 0.21.x (Nightly Channel), 0.20.x (Developer Channel) Aug 1, 2017
@luixxiul luixxiul added the help wanted The PR/issue opener needs help to complete/report the task. label Aug 8, 2017
@luixxiul luixxiul closed this Aug 8, 2017
@luixxiul luixxiul removed this from the 0.21.x (Nightly Channel) milestone Aug 8, 2017
@luixxiul
Copy link
Contributor Author

luixxiul commented Aug 8, 2017

Abandoned.

@luixxiul luixxiul removed help wanted The PR/issue opener needs help to complete/report the task. PR/blocked usability labels Aug 8, 2017
NejcZdovc pushed a commit that referenced this pull request Aug 8, 2017
NejcZdovc pushed a commit that referenced this pull request Aug 8, 2017
@luixxiul luixxiul removed the request for review from NejcZdovc August 8, 2017 14:23
@luixxiul luixxiul removed their assignment Aug 8, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants