Skip to content
This repository has been archived by the owner on Jun 24, 2022. It is now read-only.

Hotfix/v1.0.1 #1252

Merged
merged 6 commits into from
Aug 13, 2021
Merged

Hotfix/v1.0.1 #1252

merged 6 commits into from
Aug 13, 2021

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Aug 13, 2021 •
edited
Loading

Summary

Closes #1151

Light mode:
image

Dark mode:
image

Out of scope

  • Add a header to navigate to the home
  • Nicer styles, we should do in develop a new PR using a cow Meme

To Test

I've made the Cow Game page to throw an error so we can see the app crashing.

  1. Try to play the game (use the menu to navigate)
  2. Observe the error page, make sure there's no traces of Uniswap

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: Gnosis Protocol v2 Swap UI

@elena-zh
Copy link

Hey @anxolin , just like an enhancement: could we add a label app:Cowswap + Critical and something like crash report or so in order to track these issues on the board better?

Nevertheless, changes LGTM!

@alongoni
Copy link
Contributor

alongoni commented Aug 13, 2021 •
edited
Loading

Looks good! @anxolin
What about if we include the header in the error page. So users can go back to the swap page or use the menu.
image

Github and Discord links are ok!

@anxolin anxolin added Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds and removed Auto-merge PRs with this tag will be automatically merged when approved and CI succeeds labels Aug 13, 2021
@fairlighteth
Copy link
Contributor

Looks good. Same thoughts as @alongoni

fairlighteth
fairlighteth approved these changes Aug 13, 2021
View reviewed changes
Copy link
Contributor

@fairlighteth fairlighteth left a comment

Choose a reason for hiding this comment

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

Approve with a comment. Not sure if you intend to address in this PR.

fairlighteth
fairlighteth reviewed Aug 13, 2021
View reviewed changes
src/custom/components/ErrorBoundary/ErrorBoundaryMod.tsx
`

const BodyWrapper = styled.div<{ margin?: string }>`
padding: 1rem;
Copy link
Contributor

Choose a reason for hiding this comment

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

Could use px's to match other values.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a mod, i didn't touch this part. We can probably redo this component completly. For now just hotfix

fairlighteth
fairlighteth reviewed Aug 13, 2021
View reviewed changes
src/custom/components/ErrorBoundary/ErrorBoundaryMod.tsx
<LinkWrapper>
<ExternalLink
id="create-github-issue-link"
// href={`https://github.com/Uniswap/uniswap-interface/issues/new?assignees=&labels=bug&body=${encodedBody}&title=${encodeURIComponent(
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment to be removed, or?

Copy link
Contributor

Choose a reason for hiding this comment

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

its a mod.

@anxolin
Copy link
Contributor Author

anxolin commented Aug 13, 2021

@alongoni @biocom , yep, agree on that.

Maybe do that in another branch that ends up merging in develop.

My concern with your suggestion is that this is a hotfix that will go to production, and adding the normal header might introduce some issue. It can happen that the original error that triggered in the original header (maybe some hook logic)

Once this is merged in develop, we can easily iterate

W3stside
W3stside reviewed Aug 13, 2021
View reviewed changes
src/custom/pages/CowGame/index.tsx Outdated
@@ -30,6 +30,10 @@ const Wrapper = styled(Page)`
`

export default function CowGamePage() {
if (0 === 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

????

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will be reverted, its too force an error. Briefly commented in the test description

@elena-zh
Copy link

elena-zh commented Aug 13, 2021 •
edited
Loading

2 points related to the Mobile view:

  1. Is that OK that error log container is to wide?
SVID_20210813_160403_1.mp4
  1. It would be nice to increase links visibility, as they are displayed above the cow pic and almost not visible
    image
    image

@anxolin
Copy link
Contributor Author

anxolin commented Aug 13, 2021

@elena-zh good points. For this PR we will just deploy the hotfix of the issue of the repo and not being able to read.
We are in the middle of other things, and we need to deploy it to production. This issues you mentioned are already present in production. Nicer styles was removed from the scope

@elena-zh elena-zh mentioned this pull request Aug 13, 2021
Closed
elena-zh
elena-zh approved these changes Aug 13, 2021
View reviewed changes
Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

LGTM!
I have created a separate issue for the nicer styles for this page #1255

@anxolin anxolin merged commit d888a44 into master Aug 13, 2021
@W3stside W3stside deleted the hotfix/v1.0.1 branch December 16, 2021 10:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fairlighteth fairlighteth fairlighteth approved these changes

@W3stside W3stside W3stside approved these changes

@elena-zh elena-zh elena-zh approved these changes

@alongoni alongoni Awaiting requested review from alongoni

@alfetopito alfetopito Awaiting requested review from alfetopito

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@anxolin @elena-zh @alongoni @fairlighteth @W3stside