-
Notifications
You must be signed in to change notification settings - Fork 27
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
feat(editor): support using relative CSV file URLs for Gist specs #540
Conversation
src/index.ts
Outdated
@@ -14,3 +14,6 @@ export { compile } from './core/compile'; | |||
export { validateGoslingSpec } from './core/utils/validate'; | |||
export { GoslingComponent } from './core/gosling-component'; | |||
export { embed } from './core/gosling-embed'; | |||
|
|||
// Utils | |||
export { traverseTracksAndViews, traverseViewArrangements } from './core/utils/spec-preprocess'; |
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.
We could expose more util functions here later.
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.
Good to know! I wonder if we need to expose these currently to users.. We could treat these as private functions by importing from src:
// editor.tsx
import { traverseTracksAndView } from 'gosling.js/core/utils/spec-preprocess';
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 assumed those functions would be useful for users to have, but I think it might be better to expose them when we find/hear actual needs from users. Let me make changes reflecting your suggestion!
callback: (t: Partial<Track>, i: number, ts: Partial<Track>[]) => void | ||
) { | ||
if ('tracks' in spec) { | ||
spec.tracks.forEach((...args) => callback(...args)); | ||
} else { | ||
spec.tracks.forEach((t, i, ts) => { |
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.
Updates on this file are not directly related to this PR but are to just handle some cases that were not handled correctly (i.e., tracks
in a track). These two functions were not properly updated previously after we had changed the grammar.
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.
Nice! just a couple of minor comments.
src/index.ts
Outdated
@@ -14,3 +14,6 @@ export { compile } from './core/compile'; | |||
export { validateGoslingSpec } from './core/utils/validate'; | |||
export { GoslingComponent } from './core/gosling-component'; | |||
export { embed } from './core/gosling-embed'; | |||
|
|||
// Utils | |||
export { traverseTracksAndViews, traverseViewArrangements } from './core/utils/spec-preprocess'; |
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.
Good to know! I wonder if we need to expose these currently to users.. We could treat these as private functions by importing from src:
// editor.tsx
import { traverseTracksAndView } from 'gosling.js/core/utils/spec-preprocess';
editor/editor.tsx
Outdated
/** | ||
* Convert relative CSV data URLs to absolute URLs. | ||
* (e.g., './example.csv' => 'https://gist.githubusercontent.com/{urlGist}/raw/example.csv') | ||
*/ | ||
function resolveRelativeCsvUrls(spec: string) { | ||
const newSpec = JSON.parse(spec); | ||
gosling.traverseTracksAndViews(newSpec as gosling.GoslingSpec, (tv: any) => { | ||
if (tv.data && tv.data.type === 'csv' && tv.data.url.indexOf('./') === 0) { | ||
tv.data.url = tv.data.url.replace('./', `https://gist.githubusercontent.com/${urlGist}/raw/`); | ||
} | ||
}); | ||
return stringify(newSpec); | ||
} | ||
|
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 wonder if we could make this an even more reusable:
/**
* Convert relative CSV data URLs to absolute URLs.
* (e.g., './example.csv' => 'https://gist.githubusercontent.com/{urlGist}/raw/example.csv')
*/
function resolveRelativeCsvUrls(spec: string, importMeta: URL) {
const newSpec = JSON.parse(spec);
// https://regex101.com/r/l87Q5q/1
const relativePathRegex = /^[.\/]|^\.[.\/]|^\.\.[^\/]/
gosling.traverseTracksAndViews(newSpec as gosling.GoslingSpec, (tv: any) => {
if (tv.data&& tv.data.type === 'csv' && relativePathRegex.test(tv.data.url)) {
tv.data.url = new URL(tv.data.url, importMeta).href;
}
});
return stringify(newSpec);
}
and then:
const specUrl = new URL(`https://gist.githubusercontent.com/${gist}/raw/${dataFile}`);
const whenCode = fetch(specUrl.href).then(async response =>
response.status === 200 ? resolveRelativeCsvUrls(await response.text(), specUrl) : null
);
the second argument to URL
is a base
to resolve a path relative to.
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.
Thank you for this suggestion, I really like it! Good to know that URL
supports the relative URL using the second parameter. I will make the changes reflecting your comment.
This PR allows Editor users to specify relative file URLs of CSV data in Gist specs. This will work when files are located in the same Gist where the spec (i.e.,
gosling.js
) is located.File structure:
Original spec in Gist (
gosling.js
):The spec that is updated in the Editor before compiling: