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

Utilizes ModalBase to lock scroll on mobile filters #5659

Merged
merged 1 commit into from
Jun 2, 2020

Conversation

dzucconi
Copy link
Member

Unexciting :) most of the nice things in the ModalBase like the focus management would apply more to desktop uses but this at least does a better job of locking the scrolling on mobile.

Also snuck in tapping on filter to scroll to top since that's expected from a UX perspective but wouldn't work due to the modal context.


@dzucconi dzucconi requested review from damassi and anandaroop May 27, 2020 20:18
@dzucconi
Copy link
Member Author

Just realizing that button for "Filter" will look a bit weird on Desktop if this view ever happens to be triggered there. We really need an un-styled button primitive in palette — like the equivalent of TouchableWithoutFeedback. I'll PR one.

@ArtsyOpenSource
Copy link

Fails
🚫

Danger failed to run dangerfile.ts.

Error RangeError

Maximum call stack size exceeded
RangeError: Maximum call stack size exceeded
    at Object.parsePropertyName (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:2438:23)
    at Object.parseObjectMember (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:9291:10)
    at Object.parseObj (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:9225:25)
    at Object.parseExprAtom (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:8855:28)
    at Object.parseExprAtom (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:3609:20)
    at Object.parseExprSubscripts (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:8483:23)
    at Object.parseMaybeUnary (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:8463:21)
    at Object.parseExprOps (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:8329:23)
    at Object.parseMaybeConditional (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:8302:23)
    at Object.parseMaybeAssign (/home/circleci/project/node_modules/@babel/core/node_modules/@babel/parser/lib/index.js:8249:21)

Dangerfile

---------------------------^

Generated by 🚫 dangerJS against 7337b41

Copy link
Member

@anandaroop anandaroop left a comment

Choose a reason for hiding this comment

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

Whoa, ModalBase looks handy 😄

@dzucconi dzucconi force-pushed the feat/modal-base-action-sheet branch from 7337b41 to cec9e1a Compare May 29, 2020 20:22
@anandaroop
Copy link
Member

LGTM save for the odd little snapshot failure in CI

@dzucconi dzucconi force-pushed the feat/modal-base-action-sheet branch from cec9e1a to 8e7872e Compare June 2, 2020 14:15
@artsy-peril artsy-peril bot merged commit 6b6bd0f into artsy:master Jun 2, 2020
@artsy-peril artsy-peril bot mentioned this pull request Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants