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

fix: update overlay styles when switching address #180

Merged
merged 37 commits into from
Oct 10, 2023

Conversation

patricio0312rev
Copy link
Contributor

[Galleries] Address Switching can break page

Summary

image

Steps to reproduce

  1. Run the app in local mode.
  2. Make sure you have php artisan horizon running.
  3. Connect a wallet with NFTs but don't sign anything.
  4. Try to create a gallery, and select a pair of NFTs.
  5. Before clicking on Publish, change your wallet in Metamask to one without NFTs and see the magic ✨

Checklist

  • I checked my UI changes against the design and there are no notable differences
  • I checked my UI changes for any responsiveness issues
  • I checked my (code) changes for obvious issues, debug statements and commented code
  • I opened a corresponding card on Clickup for any remaining TODOs in my code
  • I added a short description on how to test this PR (if necessary)
  • I added a storybook entry for the component that was added (if necessary)
  • Documentation (if necessary)
  • Tests (if necessary)
  • Ready to be merged

@patricio0312rev patricio0312rev marked this pull request as ready for review October 3, 2023 13:15
@github-actions github-actions bot added the bugfix label Oct 3, 2023
@ItsANameToo ItsANameToo added this to the 0.7.0 milestone Oct 3, 2023
Copy link
Contributor

@shahin-hq shahin-hq left a comment

Choose a reason for hiding this comment

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

Removed extra spaces: 5a45a4e

Copy link
Contributor

@ItsANameToo ItsANameToo left a comment

Choose a reason for hiding this comment

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

won't this affect other pages as well? 🤔 in the design we have a slight visual of the background, but with this change you won't see that anymore on those pages. E.g. compare the following:

collections page on develop
image

collections page on this pr
image

I think we should instead look for the reason why the gallery page is lacking the backdrop in the outlined scenario as opposed to making this general adjustment

@ItsANameToo ItsANameToo marked this pull request as draft October 4, 2023 15:39
@patricio0312rev
Copy link
Contributor Author

@ItsANameToo fixed this issue with the following:

  • After testing, noticed that we had two implementations of the Overlay component at the same time: one was for AuthOverlay, which updates with every change in Metamask, and the other one was NoNftOverlay, which updates with the number of nfts that the user owns.

The problem I noticed was that when we changed our wallets, the AuthOverlay got updated, causing the AuthOverlay to make the blur effect disappear from the Overlay component, even if it was in use by NoNftOverlay.

  • I saw two solutions for this:
    • I prevent the render of AuthOverlay in case the NoNftOverlay is active, this change was made using the new displayAuthOverlay prop in our LayoutWrapper component.
    • I think we can remove the blur from the layout, however, the useEffect hook that implements this logic, also manages the scrolling of the page, which could be an issue to handle if we just base on pure CSS.

Let me know what you think, for now, will move this ticket back to Ready for review. 🌟 💜

@patricio0312rev patricio0312rev marked this pull request as ready for review October 5, 2023 13:23
@patricio0312rev
Copy link
Contributor Author

Video of the solution being implemented:

Screen.Recording.2023-10-05.at.08.23.31.mov

@ItsANameToo ItsANameToo modified the milestones: 0.7.0, 0.9.0 Oct 6, 2023
@ItsANameToo ItsANameToo force-pushed the develop branch 2 times, most recently from f5eb48e to 9c50c93 Compare October 10, 2023 08:36
@ItsANameToo ItsANameToo merged commit 6623d38 into develop Oct 10, 2023
11 of 12 checks passed
@ItsANameToo ItsANameToo deleted the fix/address-switching-break-page branch October 10, 2023 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants