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(native): Fixed link url in native app #1492

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

piotrski
Copy link
Member

@stackblitz
Copy link

stackblitz bot commented Jan 27, 2023

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@netlify
Copy link

netlify bot commented Jan 27, 2023

Deploy Preview for elk-docs canceled.

Name Link
🔨 Latest commit 1874cdf
🔍 Latest deploy log https://app.netlify.com/sites/elk-docs/deploys/63d454347476e100090386d0

@netlify
Copy link

netlify bot commented Jan 27, 2023

Deploy Preview for elk-zone ready!

Name Link
🔨 Latest commit 1874cdf
🔍 Latest deploy log https://app.netlify.com/sites/elk-zone/deploys/63d454349831c70008b49091
😎 Deploy Preview https://deploy-preview-1492--elk-zone.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@@ -1,4 +1,6 @@
<script setup lang="ts">
import { env } from 'process'
import { metaProperty } from '@babel/types'
Copy link
Member

Choose a reason for hiding this comment

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

Are these imports needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, it was just ts trying to be too clever. I've already removed it.

Copy link
Member

@danielroe danielroe left a comment

Choose a reason for hiding this comment

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

I can see we might have situations like this throughout the app. Maybe a feature we can toggle on in the tauri module that forces the elk.zone hostname so we don't have to reference tauri here?

@piotrski
Copy link
Member Author

@danielroe Because it's just a browser behavior, and Tauri has no control over it. I guess the only thing we could do is to monkey patch location.origin in the native bundle so that it always returns elk.zone but I'm not a fan of that solution.

@piotrski piotrski closed this Jan 28, 2023
@piotrski piotrski reopened this Jan 28, 2023
@danielroe
Copy link
Member

danielroe commented Jan 28, 2023

I understand the need and agree. I'm suggesting that rather than check the environment variable here, we instead have a global setting (maybe in app config) that changes behaviour across everywhere in the app that accesses location.origin.

Edit: I see it's just here and in login that it matters, and we probably shouldn't change that one. I still think a global setting works better. Happy to update within this PR if you don't mind me pushing to it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: bug Something isn't working
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Copy link to post not working properly
4 participants