-
Notifications
You must be signed in to change notification settings - Fork 20
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: webwallet trpc link for popups #164
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what is different between this and the previous approach, maybe an answer to some of the questions I asked answers that
|
||
// Monitor popup state | ||
const checkClosed = setInterval(() => { | ||
if (popup.closed) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if popup.closed === true
but this.currentPopup !== popup
, aka we got another popup 🤔
I'm just thinking of the possible corner case here, not stating that is an issue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be an edge case, but theoretically this.currentPopup !== popup
should not happen in this case
popup is always the new one
thinking if this approach could be more reliable but it would requires some tests (using beta
branch)
button.style.position = "fixed" | ||
button.style.top = "-999px" | ||
button.style.left = "-999px" | ||
button.style.opacity = "0" | ||
button.style.pointerEvents = "none" // prevent click event |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these styles making any difference from the previously used webwalletBtn.style.display = "none"
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did some searching and it seems it's more reliable
- Browser Security: Some browsers treat elements with display: none as "not truly interactive" from a security perspective, which can affect how they handle popup permissions
- DOM Interaction: While display: none removes the element from the layout flow completely, position: fixed keeps it in the DOM's interactive layer
- Mobile Browsers: Particularly on mobile browsers, clicks on display: none elements might be ignored or treated differently than clicks on positioned-but-invisible elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting. I saw something like this coming, legit 👍🏼
Issue / feature description
Quick description of the issue or the feature that is implemented in the pr ie: add new transitions between screens
Changes
quick list regarding the related changes within the pr
ie:
Checklist
Please keep your pull request as small as possible. If you need to make multiple changes, please create separate pull requests for each change. Create a draft pull request if you need early feedback. This will mark the pull request as a work in progress and prevent it from being reviewed or merged until you are ready.
Please move drafts to the ready for review (regular PR) when you are ready to start the review process and other developers will pick it up from there.