-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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: when deleting a workspace offline, the layout breaks #17639
Changes from 20 commits
6930885
9a094e3
e8e90e1
d07e3b1
cfb3b76
0240f90
4da28a3
732c034
3e532f7
58a96ff
4ca7979
75ab457
9f61f84
59bcbd9
3bcbdfb
cd1f55c
2d898b1
ae1ae33
9f9d54b
b56477e
56a90a7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -34,33 +34,35 @@ function showGrowlIfOffline() { | |
} | ||
|
||
/** | ||
* @param {String} url | ||
* @param {String} [url] the url path | ||
* @param {String} [shortLivedAuthToken] | ||
* | ||
* @returns {Promise<string>} | ||
*/ | ||
function openOldDotLink(url) { | ||
/** | ||
* @param {String} [shortLivedAuthToken] | ||
* @returns {Promise<string>} | ||
*/ | ||
function buildOldDotURL(shortLivedAuthToken) { | ||
const hasHashParams = url.indexOf('#') !== -1; | ||
const hasURLParams = url.indexOf('?') !== -1; | ||
function buildOldDotURL(url, shortLivedAuthToken) { | ||
const hasHashParams = url.indexOf('#') !== -1; | ||
const hasURLParams = url.indexOf('?') !== -1; | ||
Comment on lines
-37
to
+44
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akinwale @eVoloshchak can you remind me please why is this change here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To summarise: Some of the menu items made use of the You can follow the discussion here for more details: #17310 (comment) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @akinwale Ok thank you, can you also add a Qa test which will check the deeplink works as expected, also tehre is a merge conflict, I will merge once this is resolved, thank you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @mountiny All done! |
||
|
||
const authTokenParam = shortLivedAuthToken ? `authToken=${shortLivedAuthToken}` : ''; | ||
const emailParam = `email=${encodeURIComponent(currentUserEmail)}`; | ||
const authTokenParam = shortLivedAuthToken ? `authToken=${shortLivedAuthToken}` : ''; | ||
const emailParam = `email=${encodeURIComponent(currentUserEmail)}`; | ||
|
||
const params = _.compact([authTokenParam, emailParam]).join('&'); | ||
const params = _.compact([authTokenParam, emailParam]).join('&'); | ||
|
||
return Environment.getOldDotEnvironmentURL() | ||
.then((environmentURL) => { | ||
const oldDotDomain = Url.addTrailingForwardSlash(environmentURL); | ||
return Environment.getOldDotEnvironmentURL() | ||
.then((environmentURL) => { | ||
const oldDotDomain = Url.addTrailingForwardSlash(environmentURL); | ||
|
||
// If the URL contains # or ?, we can assume they don't need to have the `?` token to start listing url parameters. | ||
return `${oldDotDomain}${url}${hasHashParams || hasURLParams ? '&' : '?'}${params}`; | ||
}); | ||
} | ||
// If the URL contains # or ?, we can assume they don't need to have the `?` token to start listing url parameters. | ||
return `${oldDotDomain}${url}${hasHashParams || hasURLParams ? '&' : '?'}${params}`; | ||
}); | ||
} | ||
|
||
/** | ||
* @param {String} url the url path | ||
*/ | ||
function openOldDotLink(url) { | ||
if (isNetworkOffline) { | ||
buildOldDotURL().then(oldDotURL => Linking.openURL(oldDotURL)); | ||
buildOldDotURL(url).then(oldDotURL => Linking.openURL(oldDotURL)); | ||
return; | ||
} | ||
|
||
|
@@ -69,11 +71,11 @@ function openOldDotLink(url) { | |
API.makeRequestWithSideEffects( | ||
'OpenOldDotLink', {}, {}, | ||
).then((response) => { | ||
buildOldDotURL(response.shortLivedAuthToken).then((oldDotUrl) => { | ||
buildOldDotURL(url, response.shortLivedAuthToken).then((oldDotUrl) => { | ||
Linking.openURL(oldDotUrl); | ||
}); | ||
}).catch(() => { | ||
buildOldDotURL().then((oldDotUrl) => { | ||
buildOldDotURL(url).then((oldDotUrl) => { | ||
Linking.openURL(oldDotUrl); | ||
}); | ||
}); | ||
|
@@ -92,6 +94,7 @@ function openExternalLink(url, shouldSkipCustomSafariLogic = false) { | |
} | ||
|
||
export { | ||
buildOldDotURL, | ||
openOldDotLink, | ||
openExternalLink, | ||
}; |
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.
That is a weird bug.
Thanks for coming up with a fix so fast, but I wonder why this is happening and how could we fix this without applying workarounds.
I'll try to investigate this further a bit before we can merge this
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.
Sounds good. I have an additional fix incoming for this issue: #17656. Just need to verify the details first.