-
Notifications
You must be signed in to change notification settings - Fork 64
Select Catalog UI for the STAC Browser #990
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
base: main
Are you sure you want to change the base?
Conversation
martinRenou
left a comment
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.
Thanks for starting this!! I have some rough suggestions concerning CSS styling
| input.type = 'url'; | ||
| input.placeholder = 'https://example.com'; | ||
| input.style.width = '100%'; | ||
| input.style.padding = '8px'; | ||
| input.style.boxSizing = 'border-box'; | ||
| input.style.border = '1px solid #ccc'; | ||
| input.style.borderRadius = '4px'; | ||
| input.style.marginBottom = '20px'; |
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.
Please introduce CSS classes for those and add them in https://github.com/geojupyter/jupytergis/blob/main/packages/base/style/tabPanel.css
| catalogInputLabel.style.marginBottom = '8px'; | ||
| catalogInputLabel.style.fontWeight = 'normal'; | ||
| catalogInputLabel.style.fontSize = '0.9em'; | ||
| catalogInputLabel.style.color = '#666'; |
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.
In order to support JupyterLab themes properly (light, dark or custom themes) you'll need to make sure to use CSS variables see e.g. https://github.com/geojupyter/jupytergis/blob/main/packages/base/style/symbologyDialog.css#L114
Also things like font sizes are generalized accross lab e.g. https://github.com/geojupyter/jupytergis/blob/main/packages/base/style/symbologyDialog.css#L25, you can use var(--jp-ui-font-size2), var(--jp-ui-font-size1) etc depending on how you want it to look like.
mfisher87
left a comment
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.
Great start! You're on the right track :)
| ```{note} | ||
| Micromamba is a lightweight package manager compatible with conda environments. | ||
| It is recommended for setting up the JupyterGIS development environment. If you don’t have it installed, please follow the official documentation: [Micromamba Installation Guide](https://mamba.readthedocs.io/en/latest/installation/micromamba-installation.html) | ||
| ``` |
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.
Can you do a rebase to remove commit 3d303d2 from this PR?
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.
Instead of adding this to the repository, let's fetch it from where it lives at https://stacindex.org/api/catalogs. Once we do this, we can remove the tsconfig changes as well, and hopefully then the build will pass.
We should write this as an effect, similar to useStacSearch but much simpler. Perhaps something like (pseudocode, and I'd be happy to pair program on the implementation with you!):
const useStacIndex = (model: IJupyterGISModel): {json: string, isLoading: boolean} => {
const [isLoading, setIsLoading] = useState(true);
const [results, setResults] = useState<IStacIndexCatalogs>();
try {
const json = (await fetchWithProxies(
'https://stacindex.org/api/catalogs',
model,
async response => await response.json(),
undefined,
'internal',
)) as IStacIndexCatalogs;
setResults(json);
} catch (error) {
console.error('Error fetching data:', error);
setResults([]);
} finally {
setIsLoading(false);
}
return {json: results, isLoading};
}You'll need to define an interface type IStacIndexCatalogs that corresponds to the structure of the API response for this to work :)
We can also use the isLoading state to display a LoadingIcon during the fetch!
There's also the option of bringing in a new dependency like tanstack-query to handle data fetching, which is what the React docs recommend, but perhaps there's a reason we haven't done that yet. @martinRenou ?
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.
Instead of having the front-end request this dynamically, should we download the catalog at build time? Similar to how we build the raster layers catalog during the build.
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.
My first instinct is to fetch this data on-the-fly. I think it'd be nice to provide the latest version and it's not a large download at 52K. What's your rationale for caching it at build-time?
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's your rationale for caching it at build-time?
Making it independent of the internet connection. But making JupyterGIS fully internet independent is an impossible goal anyway so we can probably discard that.
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 like independence of an Internet connection as a design principle! We could add it to our "What is JupyterGIS" page. But what are the boundaries? I think for a STAC browser, there's an inherent need for an Internet connection to actually access the data indexed by the STAC catalogs.
98fa67a to
a0e9412
Compare
|
Integration tests report: appsharing.space |
| const StacPanel = ({ model }: IStacViewProps) => { | ||
| const inputRef = React.useRef<HTMLInputElement | null>(null); | ||
|
|
||
| const { catalogs } = useStacIndex(model); |
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.
| const { catalogs } = useStacIndex(model); | |
| const { catalogs, isLoading, error } = useStacIndex(model); |
Please use loading state to display a message while we're waiting for the catalog to load! Loading... is fine. But if you want you can use our loading component!
| </TabsContent> | ||
| </Tabs> | ||
| <div className="Select Catalog"> | ||
| <Button onClick={handleOpenDialog}>Select a Catalog</Button> |
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.
Let's add a "Selected catalog: ..." display here that displays whatever catalog the user selected!
| const catalogInputLabel = document.createElement('label'); | ||
| catalogInputLabel.textContent = 'Selected Catalog URL:'; | ||
| catalogInputLabel.style.display = 'block'; | ||
| catalogInputLabel.style.marginBottom = '8px'; | ||
| catalogInputLabel.className = 'jgis-stac-catalog-input-label'; | ||
|
|
||
| const catalogInput = document.createElement('input'); | ||
| catalogInput.type = 'url'; | ||
| catalogInput.placeholder = 'Selected catalog URL will appear here'; | ||
| catalogInput.className = 'jgis-stac-catalog-input'; | ||
| catalogInput.readOnly = true; |
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.
Let's remove this input so we have only one input and one select element in this dialog. When the select is changed, it will replace the content in the input.
| const catalogInputLabel = document.createElement('label'); | |
| catalogInputLabel.textContent = 'Selected Catalog URL:'; | |
| catalogInputLabel.style.display = 'block'; | |
| catalogInputLabel.style.marginBottom = '8px'; | |
| catalogInputLabel.className = 'jgis-stac-catalog-input-label'; | |
| const catalogInput = document.createElement('input'); | |
| catalogInput.type = 'url'; | |
| catalogInput.placeholder = 'Selected catalog URL will appear here'; | |
| catalogInput.className = 'jgis-stac-catalog-input'; | |
| catalogInput.readOnly = true; |
| const dialog = new Dialog<boolean>({ | ||
| title: 'Add Catalog', | ||
| body: widget, | ||
| buttons: [Dialog.cancelButton(), Dialog.okButton({ label: 'Add' })], |
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.
| buttons: [Dialog.cancelButton(), Dialog.okButton({ label: 'Add' })], | |
| buttons: [Dialog.cancelButton(), Dialog.okButton({ label: 'Select' })], |
|
@martinRenou the request to fetch catalog data fails on JupyterLite. Does the proxy not work in JupyterLite? I figured we had an escape hatch if there is no running proxy service. Do we have any issue to document that? I also can't browse the GEODES catalog on JupyterLite. |
Description
Checklist
Resolves #XXX.Failing lint checks can be resolved with:
pre-commit run --all-filesjlpm run lint📚 Documentation preview: https://jupytergis--990.org.readthedocs.build/en/990/
💡 JupyterLite preview: https://jupytergis--990.org.readthedocs.build/en/990/lite