feat: vertically stacked permits #274
Conversation
|
@0x4007 Still working on it. Here is how it looks currently: |
|
@0x4007 Some tests are failing on |
|
hi @pbkompasz, the tests are the expected behaviour buddy Your first run 3 days ago passed and all other runs pass just fine. Can you link to the run which has failed as I cannot seem to find it. The most recent cypress run passed too. Maybe you meant that it was CI that had failed on |
Unused files (1)
|
|
@Keyrxng I saw that the tests passed in the CI but when I ran them locally, some of the e2e test cases in |
There was a problem hiding this comment.
Overall great work with this
After making a few changes to get things working (addressed in comments) I manually QA'd with two permits from different networks and things work but there a few bugs although they can be addressed with other issues once merged (since multi-network permits aren't in use right now)
Sorry for the slow review it slipped my mind, please ping me after a day or two in the future if it happens again.
|
The only thing that bothers me about this pull is how the page renders upon loading. I spent a lot of time to refine the animation for when the permit table renders, and with this new version, it looks like it jumps around quite a bit and the animations are not smooth. However, this is not in scope for the current task so we can create a new task to refine this particular problem. |
|
@Keyrxng Fixed the bugs. |
There was a problem hiding this comment.
I think this is good to merge following any other review comments
- Dual network permits work (I can post a video if needed)
- permits are stacked and all functions work
- Manually QA'd mobile and the only comment is that you only see one permit above the fold and it's not immediately clear that you can scroll or that you have more than one
- The known issue with MM browser and us switching the user network is still present so while multi-network permits work on mobile they need to change their network manually. (For mobile QA I did not fire any txs but I did receive the relevant wallet prompts for them. That's because it's difficult to work with anvil and mm mobile)
|
Seems fine to me, good to go when you fix |
|
@gentlementlegen done |
|
The styles are quite messed up. Make sure to check where you are setting font colors, and to match how it's done elsewhere so that it will work in light mode automatically There is also a new line that shouldn't be there
Tested what should be the latest commit from the CI/CD |
| } | ||
| @media only screen and (max-width: 600px) { | ||
| table { | ||
| max-width: 90%; |
|
@0x4007 Fixed light mode. |
|
Gonna need to test this again on desktop. |
Resolves #196