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

Fix vertical spacing for all navbar item combinations #10512

Merged
merged 1 commit into from
Sep 4, 2017

Conversation

petemill
Copy link
Member

@petemill petemill commented Aug 15, 2017

Fix #9370 by using collapsible margin so that items without border+padding can be evenly spaced (caption buttons, bookmark, tab pages), and those with border+padding (notifications, tabs) can be touching. Removes empty element for tab pages when there is only a single page, as it had no functional purpose. Instead, margin is used.

Buttons, Notification, Bookmarks, Tab Pages, Tabs, Find

screenshot 2017-08-15 13 06 42

Buttons, Notification, Bookmarks, Tab Pages, Tabs

screenshot 2017-08-15 13 07 00

Buttons, Notification, Tab Pages, Tabs

screenshot 2017-08-15 13 07 19

Buttons, Notification, Bookmarks, Tabs

screenshot 2017-08-15 13 09 54

Buttons, Bookmarks, Tab Pages, Tabs

screenshot 2017-08-15 13 07 50

Buttons, Tab Pages, Tabs

screenshot 2017-08-15 13 08 06

Buttons, Tabs

screenshot 2017-08-15 13 09 02

Buttons, Bookmarks, Tabs

screenshot 2017-08-15 13 09 17

Buttons, Tabs, Find

screenshot 2017-08-15 13 18 45

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 Plan:

  • Enable above combinations and check visual regressions compared to screenshots.

Draggable area regression checks (inspired by #7740)

Test case 1 - default status

  1. Clear your profile
  2. Start the browser
  3. Make sure that the area between the URL bar and the tabs bar is draggable

Test case 2 - text only

  1. Open https://github.com/ in a new tab
  2. Open https://github.com/features in a new tab
  3. Open https://github.com/explore in a new tab
  4. Open https://github.com/pricing in a new tab
  5. Display the bookmark toolbar
  6. Bookmark them by dragging the lock icon on the URL bar to the bookmark toolbar
  7. (Not on Windows) Make sure that the area between the bookmarked items and the URL bar is draggable
  8. (Not on Windows) Make sure that the area between the bookmarked items and the tabs bar is draggable

Test case 3 - text only with tab sets

  1. Open about:preferences#tabs
  2. Set the number of tabs per tab set to 6
  3. Open several new tabs to create another tab page
  4. Narrow the window size to place the tab page indicators under the bookmarked items
  5. (Not on Windows) Make sure that the area between the tab page indicators and the bookmark bar is draggable (the tiny area between the indicators is NOT draggable)
  6. Make sure that the area between the tab page indicators and the tabs bar is draggable

Test case 4 - text and favicons with tab sets

  1. Open about:preferences
  2. Change "Text only" to "Text and Favicons"
  3. Repeat the step 3:5 and 3:6
  4. Close several tabs to clear the 2nd tab set
  5. (Not on Windows) Make sure that the area around the bookmarks is draggable

Test case 5 - only tab sets

  1. Hide the bookmark toolbar
  2. Make sure that the area around the tab page indicators is draggable

Test case 6 - favicon only with and without tab sets

  1. Bookmark several other GitHub pages
  2. Open about:preferences
  3. Change "Text and Favicons" to "Favicon only"
  4. Change window size to place the tab page indicators under the bookmark favicons
  5. Repeat the step 3:5 and 3:6
  6. Close tabs to clear the 2nd tab set
  7. (Not on Windows) Make sure that the area around favicons is draggable

Test case 7 - main menu button (especially Windows)

  1. Open a small amount of tabs so there is space between tabs and main menu button
  2. Ensure dragging the space between tabs and main menu button does drag the window
  3. Ensure clicking the main menu button opens the main menu

Test case 8 - notification bar

  1. Click 'check for updates...' menu item
  2. Make sure that nowhere on the notification bar is draggable

Test case 9 - find page bar

  1. Visit github.com
  2. Open 'find in page' bar using cmd-f / ctrl-f or the menu item
  3. Make sure no area of 'find in page' bar is draggable

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

@bsclifton bsclifton added this to the 0.21.x (Nightly Channel) milestone Aug 15, 2017
@luixxiul
Copy link
Contributor

@petemill it looks great! If you don't mind, would you specify the manual test plan, like I did on #7740 (comment) ? You could re-use the test plan, adding another test case with the update bar.

We'd need to make sure this PR does not introduce regressions regarding draggability :-) thanks!

}
</div>
{
this.props.isSinglePage
Copy link
Contributor

Choose a reason for hiding this comment

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

nice one ;-)

@luixxiul luixxiul added design A design change, especially one which needs input from the design team. feature/navbar feature/tabsbar labels Aug 16, 2017
@petemill petemill force-pushed the navbar-vertical-rhythm branch 3 times, most recently from e7c03ae to fdaf601 Compare August 23, 2017 22:24
Fix brave#9370 by using collapsible margin so that items without border+padding can be evenly spaced (caption buttons, bookmark, tab pages), and those with border+padding (notifications, tabs) can be touching. Removes empty element for tab pages when there is only a single page, as it had no functional purpose. Instead, margin is used.
@petemill
Copy link
Member Author

@luixxiul I added the test cases concerning window draggability, ran through those cases on mac and windows, and made some fixes as a result. Ready for your review.

@NejcZdovc
Copy link
Contributor

@luixxiul can you please re-review? Thank you

Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

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

The test plan works and changes LGTM.

@bsclifton bsclifton merged commit 36bf92d into brave:master Sep 4, 2017
@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
petemill added a commit that referenced this pull request Jan 17, 2018
Previously it was enough to not add 'drag' to the bookmarks bar on Windows, but not that it is added at a higher level in the DOM (so that dragging works in the margin between the bars) via #10512, we have to explicitly set it to 'no-drag' on each child.

Fix #11728
petemill added a commit that referenced this pull request Jan 17, 2018
Previously it was enough to not add 'drag' to the bookmarks bar on Windows, but now that it is added at a higher level in the DOM (so that dragging works in the margin between the bars) via #10512, we have to explicitly set it to 'no-drag' on each child.

Fix #11728
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
design A design change, especially one which needs input from the design team. feature/navbar feature/tabsbar
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants