Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed menu width #676

Merged
merged 7 commits into from Apr 9, 2019

Conversation

Projects
None yet
4 participants
@bpierre
Copy link
Member

bpierre commented Apr 4, 2019

Makes the menu width fixed even in auto closing mode:

Since the area where the app is visible is much larger, the overlay has also been made visible.

Also fixes a bug at 768px, where the menu was opened but couldn’t be closed (due to a mismatch between CSS media queries and Viewport).

@bpierre bpierre requested review from sohkai and dizzypaty Apr 4, 2019

@bpierre bpierre requested a review from 2color Apr 5, 2019

@2color

2color approved these changes Apr 8, 2019

@sohkai

sohkai approved these changes Apr 8, 2019

Copy link
Member

sohkai left a comment

LGTM, some small comments


const THRESHOLD_VERTICAL_TOLERANCE = 10
const THRESHOLD_DIRECTION = 0.2
const THRESHOLD_PROGRESS = 0.5
const MENU_WIDTH = 220

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Maybe we could export this as part of MenuPanel?

* from the edge of their screen when an iframe app is being
* used */
width: progress.interpolate(p =>
p === 0 ? '10px' : '100vw'

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Should we update the comment to 10px? Is the increase to make the swipe register more consistently?

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 8, 2019

Author Member

ooooh this was for debugging purposes, good catch 😮

@@ -234,7 +234,7 @@ class Wrapper extends React.PureComponent {
connected={connected}
notifications={notifications.length}
daoAddress={daoAddress}
openProgress={progress}
swipeProgress={progress}
autoClosing={autoClosingPanel}

This comment has been minimized.

Copy link
@sohkai

sohkai Apr 8, 2019

Member

Wondering if we can roll autoClosingPanel and menuSwipeEnabled into the same prop, perhaps collapsibleMenuBar?

This comment has been minimized.

Copy link
@bpierre

bpierre Apr 8, 2019

Author Member

Agreed, especially since the swipe feature doesn’t only depend on the viewport’s width.

See 4e5aa06

bpierre added some commits Apr 9, 2019

Remove menuSwipeEnabled from Wrapper
Enabling the swipe gesture is a decision made by SwipeContainer, which
is why its `enabled` prop has also been renamed to remove any confusion.

@bpierre bpierre merged commit ae2d903 into master Apr 9, 2019

1 of 3 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details
license/cla Contributor License Agreement is signed.
Details

@bpierre bpierre deleted the menu-fixed-width branch Apr 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.