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

Removes unnecessary width/height #7661

Merged
merged 1 commit into from
Apr 3, 2017
Merged

Removes unnecessary width/height #7661

merged 1 commit into from
Apr 3, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Mar 12, 2017 •
edited by bsclifton
Loading

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

Resolves #7403 #7662 #1589

Auditors

@bsclifton

Test Plan

  • see each issue for detailed steps

@NejcZdovc NejcZdovc added this to the 0.13.6 milestone Mar 12, 2017
@NejcZdovc NejcZdovc self-assigned this Mar 12, 2017
@NejcZdovc NejcZdovc requested a review from bsclifton March 12, 2017 10:59
@NejcZdovc NejcZdovc removed the request for review from bsclifton March 12, 2017 17:46
@NejcZdovc NejcZdovc force-pushed the hotfix/#7403-menubar branch 2 times, most recently from 33c3b2c to 7d1791c Compare March 12, 2017 19:43
@NejcZdovc NejcZdovc requested a review from bsclifton March 12, 2017 19:43
@NejcZdovc NejcZdovc force-pushed the hotfix/#7403-menubar branch from 7d1791c to aa6adeb Compare March 12, 2017 22:06
@luixxiul
Copy link
Contributor

@NejcZdovc does this close #1589 too?

@NejcZdovc
Copy link
Contributor Author

@luixxiul yeah I think it does

@NejcZdovc NejcZdovc force-pushed the hotfix/#7403-menubar branch from aa6adeb to 0628135 Compare March 13, 2017 06:21
@luixxiul
Copy link
Contributor

@NejcZdovc Sweet :-)

bsclifton
bsclifton suggested changes Mar 13, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I've confirmed all 3 bugs are fixed with this change 😄 I tested on a Windows 10 VM

However, I did notice overflow was shown using a scrollbar:
image

We may want to set an overflow-y with hidden and see if that helps

@NejcZdovc
Copy link
Contributor Author

@bsclifton what do you mean exactly with the overflow?

@bsclifton
Copy link
Member

@NejcZdovc the screenshot doesn't capture it very well, but the div itself is overflowing on the Y axis which causes the scrollbar to be shown to the right of the context menu

@NejcZdovc
Copy link
Contributor Author

Yes scroll bar should be there and overflow-y: hidden will prevent scrolling. Should we add a custom scroll bar?

cc @bradleyrichter

@bsclifton
Copy link
Member

Moving back to 0.13.7 because of the design feedback needed

@bsclifton bsclifton modified the milestones: 0.13.7, 0.13.6 Mar 13, 2017
@cezaraugusto
Copy link
Contributor

I think this case could benefit from using scrollbar pseudo-element since we're on Webkit

@NejcZdovc
Copy link
Contributor Author

@bsclifton @cezaraugusto @bradleyrichter I would suggest that for this version 0.14.1 we add webkit scroller (as suggested by @cezaraugusto) on the left side of the root context menu. For the next release we do a complete rewrite of a context menu to the macOS way and some more improvements. What do you think?

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Mar 26, 2017 •
edited
Loading

@bsclifton @cezaraugusto Based on @cezaraugusto suggestion I used pseudo-element, but instead of adding a custom css, I just hide it (made transparent), so now we have the same behavior that we have in the current version, but we also resolve all problems that this PR wants to solve. What do you think?

@NejcZdovc NejcZdovc force-pushed the hotfix/#7403-menubar branch from 0628135 to 5fd38e0 Compare March 26, 2017 10:54
@NejcZdovc
Removes unnecessary width/height
b781c16 
Resolves brave#7403
Resolves brave#7662
Resolves brave#1589

Auditors: @bsclifton

Test Plan:
- specified in the issue brave#7403
@NejcZdovc NejcZdovc force-pushed the hotfix/#7403-menubar branch from 5fd38e0 to b781c16 Compare March 26, 2017 11:21
bsclifton
bsclifton approved these changes Apr 3, 2017
View reviewed changes
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.

Confirmed all 3 cases look good! Great job 😄

@bsclifton
Copy link
Member

I updated all 3 issues to have more detailed test plan; feel free to edit/adjust, @NejcZdovc 😄

@bsclifton bsclifton merged commit 0aece94 into brave:master Apr 3, 2017
@srirambv srirambv mentioned this pull request Apr 4, 2017
Closed
@NejcZdovc NejcZdovc mentioned this pull request Apr 4, 2017
Closed
@ghost ghost mentioned this pull request Apr 4, 2017
Closed
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bsclifton bsclifton bsclifton approved these changes

@cezaraugusto cezaraugusto Awaiting requested review from cezaraugusto

Assignees

@NejcZdovc NejcZdovc

Labels
accessibility feature/context-menu QA/test-plan-specified release-notes/include
Projects
None yet
Milestone
0.15.0
Development

Successfully merging this pull request may close these issues.
4 participants
@NejcZdovc @luixxiul @bsclifton @cezaraugusto