Add support for layoutUrl as a param #341
Conversation
|
This PR has been marked as stale because there has been no activity in the past 3 months. Please add a comment to keep it open. |
|
Waiting for feedback from any contributor... |
|
Hey @charlielito when possible update this branch too to make the pipelines run! Cheers, |
|
So in the meantime I tested the branch on your fork and couldn't make it work, I hosted the layouts myself and both |
| log.debug(`Could not load the layout from ${layoutUrl}`); | ||
| return; | ||
| } | ||
| const parsedState: unknown = JSON.parse(await res.text()); |
There was a problem hiding this comment.
I'd change this to type LayoutData instead and put it inside a try and catch in case something goes wrong with the parsing.
| } | ||
| const parsedState: unknown = JSON.parse(await res.text()); | ||
|
|
||
| if (typeof parsedState !== "object" || !parsedState) { |
There was a problem hiding this comment.
With parsedState being type of LayoutData the second condition being verified isn't necessary here
| return; | ||
| } | ||
|
|
||
| const layoutData = parsedState as LayoutData; |
There was a problem hiding this comment.
Assertion wont be necessary here if suggestion above is implemented
| return; | ||
| } | ||
|
|
||
| let shouldUpdate; |
There was a problem hiding this comment.
I'd initialize this variable as false to avoid eslint[@typescript-eslint/strict-boolean-expressions](https://typescript-eslint.io/rules/strict-boolean-expressions) on line 638
ctw-joao-luis
left a comment
ctw-joao-luis
left a comment
There was a problem hiding this comment.
Left some suggestions
| if (!layoutUrl) { | ||
| return; | ||
| } | ||
| let res; |
There was a problem hiding this comment.
I'd rename it to response and make it const.
if (!layoutUrl) {
return;
}
try {
const response = await fetch(layoutUrl);
if (!response.ok) {
log.debug(`Failed to fetch layout: ${layoutUrl} (status ${response.status})`);
return;
}
const parsed: unknown = JSON.parse(await response.text());
if (!parsed || typeof parsed !== "object") {
log.debug(`${layoutUrl} does not contain valid layout JSON`);
return;
}
const layoutData = parsed as LayoutData;
setCurrentLayout({ data: layoutData });
} catch (error) {
log.debug(`Could not load the layout from ${layoutUrl}`, error);
}| if ("layoutUrl" in urlState) { | ||
| if (urlState.layoutUrl) { | ||
| newURL.searchParams.set("layoutUrl", urlState.layoutUrl); | ||
| } else { | ||
| newURL.searchParams.delete("layoutUrl"); | ||
| } | ||
| } |
There was a problem hiding this comment.
| if ("layoutUrl" in urlState) { | |
| if (urlState.layoutUrl) { | |
| newURL.searchParams.set("layoutUrl", urlState.layoutUrl); | |
| } else { | |
| newURL.searchParams.delete("layoutUrl"); | |
| } | |
| } | |
| if ("layoutUrl" in urlState) { | |
| urlState.layoutUrl | |
| ? newURL.searchParams.set("layoutUrl", layoutUrl) | |
| : newURL.searchParams.delete("layoutUrl"); | |
| } |
There was a problem hiding this comment.
Please, implement some unit tests in appURLState.test.ts to cover your changes
|
@charlielito gentle reminder to implement unit test in appURLState.test.ts to cover your changes. |
|
Is this still in progress? It would be a nice feature to have |
|
I'll take a look |
@charlielito I could support if you do not find the time. Please let me know. |
6 participants
User-Facing Changes
Added the option to pass a URL to a layout file
Description
Just before Foxglove went close source, we saw a lot of users asking for a way to preload a layout in the URL for easy sharing of configuration between people just with the URL. A user at that time did a PR adding this functionality but they declined it. We ported this to our fork and it is very helpful inside our team.
Now we would like this to be part of the open-source platform that remains. Since our fork is from when Foxglove was open source days we had to port to your version which has changed.
If you think this would be nice to be merged I'd appreciate some input on this PR since it doesn't work, somehow the layout is not updated correctly. What am I missing?
Note: for this to work, you need to enable the CORS policy wherever you store your layouts JSON files so the browser can get them.
I enabled this temporarily on some data so you can debug it if you want locally at: http://localhost:8080/?layoutUrl=https%3A%2F%2Fstorage.googleapis.com%2Fgcp-io-tests%2Fexample_layout.json
Checklist