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

Follow-up of #7547 (Update the top panel) #7740

Merged
merged 2 commits into from
Mar 20, 2017
Merged

Follow-up of #7547 (Update the top panel) #7740

merged 2 commits into from
Mar 20, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Mar 15, 2017

Closes #7739

Auditors:

Test Plan:

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

screenshot 2017-03-16 11 47 29

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

screenshot 2017-03-16 11 56 11

Test case 3 - text only with tab sets
12. Open about:preferences#tabs
13. Set the number of tabs per tab set to 6
14. Open several new tabs to create another tab page
15. Narrow the window size to place the tab page indicators under the bookmarked items
16. (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)
17. Make sure that the area between the tab page indicators and the tabs bar is draggable

screenshot 2017-03-16 11 57 14

Test case 4 - text and favicons with tab sets
18. Open about:preferences
19. Change "Text only" to "Text and Favicons"
20. Repeat the step 16 and 17
21. Close several tabs to clear the 2nd tab set
22. (Not on Windows) Make sure that the area around the bookmarks is draggable

screenshot 2017-03-16 11 57 34

screenshot 2017-03-17 16 41 48

Test case 5 - only tab sets
23. Hide the bookmark toolbar
24. Make sure that the area around the tab page indicators is draggable

screenshot 2017-03-16 11 50 55

Test case 6 - favicon only with and without tab sets
25. Bookmark several other GitHub pages
26. Open about:preferences
27. Change "Text and Favicons" to "Favicon only"
28. Change window size to place the tab page indicators under the bookmark favicons
29. Repeat the step 16 and 17
29. Close tabs to clear the 2nd tab set
30. (Not on Windows) Make sure that the area around favicons is draggable

screenshot 2017-03-16 12 31 24 screenshot 2017-03-16 12 31 38

  • 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).

Test Plan:

Copy link
Contributor

@jamesmudgett jamesmudgett left a comment

Choose a reason for hiding this comment

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

Looking better. Think the bottom spacing on the bookmarks seems off if the page indicators are hidden, would suggest:

  • Update to have equal spacing between address bar and tabs change margin-bottom: 5px to margin-top: 5px on .bookmarksToolbar.
  • Set .tabPages padding to padding: 5px 0 5px to give space between page indicator and bookmarks bar.
  • Finally, correct spacing on address bar by setting margin-top: 7px instead of 1px and remove the height: 36px on .navigatorWrapper properties.

@jamesmudgett
Copy link
Contributor

Final result should look like this:
screen shot 2017-03-15 at 4 51 13 pm

@luixxiul luixxiul requested a review from bsclifton March 16, 2017 03:08
@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 16, 2017

Updated with some other tweaks, focusing on draggability.

Also I updated the test plan in the 1st post.

Closes #7739

Auditors:

Test Plan:

Test case 1
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
4. Open https://github.com/
5. Open https://github.com/features
6. Open https://github.com/explore
7. Open https://github.com/pricing
8. Bookmark them
9. Make sure that the area between the bookmarked items and the URL bar is draggable
10. Make sure that the area between the bookmarked items and the tabs bar is draggable

Test case 3
11. Open about:preferences#tabs
12. Set the number of tabs per tab set to 6
13. Open several new tabs to create another tab page
14. Change window size to place the tab page indicators under the bookmarked items
15. 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)
16. Make sure that the area between the tab page indicators and the tabs bar is draggable

Test case 4
17. Open about:preferences
18. Change "Text only" to "Text and Favicons"
19. Repeat the step 15 and 16

Test case 5
20. Hide the bookmark toolbar
21. Make sure that the area around the tab page indicators is draggable

Test case 6
22. Bookmark several other GitHub pages
23. Open about:preferences
24. Change "Text and Favicons" to "Favicon only"
25. Change window size to place the tab page indicators under the bookmark favicons
26. Repeat the step 15 and 16
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

I just saw this (image attached)

You can enable the bookmarks toolbar, but it doesn't take up the full space. After dragging an item to it (which is a little trickier, since you have to guess where)... it resizes itself

(edit: because it's a GIF and palette is limited to 256 colors, please ignore the dithering taking place in the image)
bm-toolbar

@luixxiul
Copy link
Contributor Author

Thanks, I'll take a look.

@luixxiul
Copy link
Contributor Author

luixxiul commented Mar 17, 2017

It is because of the margin-bottom of URL bar, specifically .navigatiorWrapper.

See: #7740 (review)

Auditors: @bsclifton

Test Plan: See the 1st post of the PR (updated)
@luixxiul
Copy link
Contributor Author

Updated. It looks hacky a bit and also it is not compatible with Aphrodite for now, still it works better.

The modification based on the feedback by @bsclifton is covered with the Test case 2 in the 1st post of this PR, which is also updated.

@luixxiul luixxiul requested a review from bsclifton March 17, 2017 08:03
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

I noticed that right border is flickering when you are resizing. For width/2+1 is working ok, but for width/2, border is smaller
8

@luixxiul
Copy link
Contributor Author

@NejcZdovc I see the same thing on master as well.

@NejcZdovc
Copy link
Contributor

@luixxiul Can you fix it in this PR as well?

@luixxiul
Copy link
Contributor Author

Sorry, I don't know how to.

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Mar 18, 2017

@luixxiul You can try something like this:

 less/navigationBar.less | 2 --
 1 file changed, 2 deletions(-)

diff --git a/less/navigationBar.less b/less/navigationBar.less
index fc6e272..25e1217 100644
--- a/less/navigationBar.less
+++ b/less/navigationBar.less
@@ -725,8 +725,6 @@
   &:not(.titleMode) {
     > * {
       animation: fadeIn .6s;
-      opacity: 0;
-      animation-fill-mode: forwards;
     }

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Changes look good to me! I tried on macOS and Windows. The only feedback I have: on Windows, that area (specified in the test steps) is not draggable. This is because it needs to be right clickable (for the context menu). Windows can't have both. Can you please update the test steps to instead maybe say "try right click on Windows" or similar?

Otherwise, looks great! 😄

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.

LGTM

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.

5 participants