Skip to content
This repository has been 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

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Mar 12, 2017

  • 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

@luixxiul
Copy link
Contributor

@NejcZdovc does this close #1589 too?

@NejcZdovc
Copy link
Contributor Author

@luixxiul yeah I think it does

@luixxiul
Copy link
Contributor

@NejcZdovc Sweet :-)

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'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

@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?

Resolves brave#7403
Resolves brave#7662
Resolves brave#1589

Auditors: @bsclifton

Test Plan:
- specified in the issue brave#7403
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 😄

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