search

gnosisguild / zodiac-safe-app

https://gnosis-safe.io/app/share/safe-app?appUrl=https://zodiac.gnosisguild.org/&chainId=5
GNU Lesser General Public License v3.0
26 stars 28 forks source link

Fix "Open Roles App" navigation #155

Closed cristovaoth closed 1 year ago

cristovaoth commented 1 year ago

https://github.com/gnosis/zodiac-safe-app/issues/154

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
zodiac-safe-app ✅ Ready (Inspect) Visit Preview Nov 1, 2022 at 9:47AM (UTC)
zodiac-safe-app-api ✅ Ready (Inspect) Visit Preview Nov 1, 2022 at 9:47AM (UTC)
netlify[bot] commented 1 year ago

Deploy Preview for gnosis-zodiac-app failed.

Name Link
Latest commit df352e624d8131fd30ce68e66f0f78f77d7ee2a2
Latest deploy log https://app.netlify.com/sites/gnosis-zodiac-app/deploys/6360e8d6e35aad0009b967b3
cristovaoth commented 1 year ago

@auryn-macmillan

auryn-macmillan commented 1 year ago
  • We can't really read from parent context, but we can assign it a value, and this will result in a full page reload. Not sure if this is still preferable to new tab?

I'm honestly indifferent in this case. Maybe we just stick with a new tab.

cristovaoth commented 1 year ago
  • We can't really read from parent context, but we can assign it a value, and this will result in a full page reload. Not sure if this is still preferable to new tab?

I'm honestly indifferent in this case. Maybe we just stick with a new tab.

But your initial expectation was for only the iframe to reload, right? I don't think this is possible (@manboy-eth thoughts?)

asgeir-s commented 1 year ago

I could have refactored the code in safeAppLink to use URL and URLSearchParams, but opted to code the new approach inline in the file since theres another usage of that safeAppLink function

Your approach looks much better than the current implementation in safeAppLink. Would love to have the new version in safeAppLink so that we only do it one way (for ease of maintenance). See the comment below (it seems like the URL manipulation is not required).

asgeir-s commented 1 year ago
  • We can't really read from parent context, but we can assign it a value, and this will result in a full page reload. Not sure if this is still preferable to new tab?

I'm honestly indifferent in this case. Maybe we just stick with a new tab.

But your initial expectation was for only the iframe to reload, right? I don't think this is possible (@manboy-eth thoughts?)

I experimented a bit with this, it seems that we can navigate to another Safe app without a full reload (only the iFrame will change location), via something like this 

window.location.href = `https://roles.gnosisguild.org/#/${currentChainShortName}:${module.owner}

So replace the use of href with onClick and do this 👆 Then it seems we won't even need any of the URL manipulations.

cristovaoth commented 1 year ago

Thanks @manboy-eth good catch. We can actually do both. Will follow up

auryn-macmillan commented 1 year ago

@samepant I think you control our Netlify account, right? Could you update the ENV there, currently the preview deploys are broken because some of the newer variables are missing.