Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

[instant] Dismissible overlay for mobile errors #1229

Merged
merged 34 commits into from
Nov 9, 2018
Merged

[instant] Dismissible overlay for mobile errors #1229

merged 34 commits into from
Nov 9, 2018

Conversation

steveklebanoff
Copy link
Contributor

@steveklebanoff steveklebanoff commented Nov 8, 2018 •
edited
Loading

Description

Shows a dark overlay over the widget when an error occurs in mobile. Upon clicking the overlay, the error is dismissed.

⚠️ Should squash & merge since there are a lot of experimental commits here ⚠️

dismissibleerrorgif

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Prefix PR title with bracketed package name(s) corresponding to the changed package(s). For example: [sol-cov] Fixed bug.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons.

@steveklebanoff
WIP conditional animation
abfeb96
@steveklebanoff
WIP: media working
1d3bdd4
@steveklebanoff
working with default
3660cf2
@steveklebanoff
working normally
9ef4f68
@steveklebanoff
proof of concept working
5c63df1
@steveklebanoff
working with own helper
a70d2aa
@steveklebanoff
Make PositionAnimation handle screen specific settings
4c640a4
@steveklebanoff
Have PositionAnimation support default and small
0a52ace
@steveklebanoff
Working with zindexs
5357454
@steveklebanoff
Merge branch 'feature/instant/mobile-full-bleed' into feature/instant…
32f444d 
…/wip-viewport-conditional-animations
@steveklebanoff
Takeout unneeded dependency
1144136
@steveklebanoff
Takeout unused prop
de5bb42
@steveklebanoff
import cleanup
750c552
@steveklebanoff
Takeout old comment and moving props closer to component
e0b2705
@steveklebanoff
Fix zIndexes
ea3e456
@steveklebanoff
Use media helper
0cd5e0e
@steveklebanoff
Make function more generic
28f47b8
@steveklebanoff
Add padding
00704b2
@steveklebanoff
Linting
9e17fca
@steveklebanoff
Show dismissable overlay when there is an error
5d0c07c
@steveklebanoff
Only show overlay on mobile devices
64246a7
@steveklebanoff
Cleanup so onClick doesn't trigger
37dfc88
@steveklebanoff
onClick, onOverlayClick
c2f2aba
@steveklebanoff
Get z-index from constant
19ab63d
@steveklebanoff
Commit new z-index
acd7f32
@steveklebanoff
Move to separate component
c6132d2
@coveralls
Copy link

coveralls commented Nov 8, 2018 •
edited
Loading

Coverage Status

Coverage remained the same at 84.721% when pulling 38ed866 on feature/instant/dismissable-mobile-errors into 9e17fca on feature/instant/viewport-specific-errors.

@steveklebanoff
Copy link
Contributor Author

Updated w/ development and ready for review

fragosti
fragosti reviewed Nov 9, 2018
View reviewed changes
Copy link
Contributor

@fragosti fragosti left a comment

Choose a reason for hiding this comment

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

Is it a bit too much to have a LatestErrorOverlay? It can't just be part of LatestError?

Also this whole experience feels kind of off to me but that's more of UX / design feedback

packages/instant/src/containers/latest_error_overlay.tsx Outdated
import { DisplayStatus } from '../types';
import { errorFlasher } from '../util/error_flasher';

type ConnectedState = Pick<OverlayContainerProps, 'showOverlay' | 'showMaxWidth'>;
Copy link
Contributor

Choose a reason for hiding this comment

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

Oh I like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yea, I think we could benefit from adopting this pattern for all of our containers. Would help reduce work when interfaces change.

packages/instant/src/components/zero_ex_instant_container.tsx Outdated
<SelectedAssetBuyOrderStateButtons />
</Container>
</Flex>
<LatestErrorOverlay>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can it just be a sibling?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, thanks for calling this out. This hierarchy was a relic of when the overlay was implemented with an :after css selector

@steveklebanoff
Copy link
Contributor Author

@fragosti thanks for the callout about this being able to be done all in LatestError -- a lot of this design originated from the first pass which was before the more generic Overlay, and was based on an :after css effect. I've updated this to just be handled in LatestError and the code feels a lot cleaner now 🎉

I think this overlay would benefit from a fade-in/fade-out, and it looks like that is the way Chris designed it in his mockup, but I would like to get this in as-is, and add the transition as a follow up step

@steveklebanoff steveklebanoff merged commit 2c585bf into development Nov 9, 2018
@adamlaska adamlaska mentioned this pull request Nov 5, 2022
Open
@adamlaska adamlaska mentioned this pull request Dec 25, 2022
Open
This was referenced Jun 20, 2023
Open
Open
Open
Open
Open
Open
This was referenced Nov 30, 2023
Open
Open
Open
Open
Open
Open
Open
Open
Open
@adamlaska adamlaska mentioned this pull request Dec 17, 2023
Open
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@BMillman19 BMillman19 BMillman19 approved these changes

@fragosti fragosti fragosti approved these changes

Assignees
No one assigned
Labels
@0x/instant
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@steveklebanoff @coveralls @BMillman19 @fragosti