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

feat: React Examples Components (DT-7024) #49

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

TheMooseman
Copy link
Contributor

@TheMooseman TheMooseman commented Jan 2, 2025

What

Create react components for our examples(excluding layers).

How

  • Create components
  • Ensure examples still work
  • Apply learnings from implementations

PR Checklist

  • Is your PR title following our conventional commit naming recommendations?
  • Have you filled in the PR Description Template?
  • Is your branch up to date with the latest in main?
  • Do the CI checks pass successfully?
  • Have you smoke tested the example applications?
  • Did you check that the changes meet accessibility standards?
  • Have you tested the application on these browsers?
    • Chrome (Fully supported)
    • Firefox (Major bug fixes supported)
    • Safari (Major bug fixes supported)

@TheMooseman TheMooseman requested a review from a team as a code owner January 2, 2025 21:41
Copy link
Collaborator

@lanesawyer lanesawyer left a comment

Choose a reason for hiding this comment

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

Routing works! Let's get the Layers app showing too, since that's an important one.

There is a type error that keeps Vite from building, easy fix though.

I'll poke at this again once it's fully baked!

<a href="/omezarr">OMEZARR</a>
<br />
</li>
{/* layers is not in the AC to be converted to a react component
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's at least let it render like it was, since there's a good Markdown page explaining how to use it in the docs still so we want it working.

@TheMooseman TheMooseman changed the title [WIP] feat: React Examples Components (DT-7024) feat: React Examples Components (DT-7024) Jan 8, 2025
@TheMooseman TheMooseman requested a review from lanesawyer January 8, 2025 22:21
Copy link
Collaborator

@lanesawyer lanesawyer left a comment

Choose a reason for hiding this comment

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

Could you remove examples/public/layers.html? Only examples/layers.html is needed.

Looking great, everything works! The demos don't render until you interact with the page, like a scroll event, and then they pop up. That would be nice to fix, or we can add a message on the screen for now.

camera={{ ...exampleSettings.camera, view }}
wheel={zoom}
/>
<div style={{ display: 'flex', flexDirection: 'row' }}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add some text to the page encouraging users to scroll to get the demos to appear? Alternatively, we figure out why it takes interaction to make these show up properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In bkp we solve that problem with an "onComplete" callback with a hook, but lets wait to look into this deeper in another ticket

@@ -63,6 +56,7 @@ export function DziView(props: Props) {
}
return () => {
if (cnvs.current) {
// eslint-disable-next-line react-hooks/exhaustive-deps
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this is out of place? This isn't disabling the useEffect dependency lint issues. We also still need to actually integrate a linter soon 😅

};

return omezarr && settings ? (
<RenderServerProvider>
Copy link
Collaborator

Choose a reason for hiding this comment

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

This might be why the omezarr demo requires interaction like a scroll to show up. Always return the provider, then do the ternary inside it as children

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that fixes it, yep!

Copy link
Collaborator

@lanesawyer lanesawyer left a comment

Choose a reason for hiding this comment

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

LGTM

const colorByGene: ColumnRequest = { name: '88', type: 'QUANTITATIVE' };
const scottpoc = 'https://tissuecyte-ome-zarr-poc.s3.amazonaws.com/40_128_128/1145081396';

export const examples: Record<string, any> = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
export const examples: Record<string, any> = {
export const examples = {

Copy link
Collaborator

Choose a reason for hiding this comment

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

dont erase types with any...

plane: 'xy',
slices: 4,
} as ZarrSliceGridConfig,
['structureAnnotation']: {
Copy link
Collaborator

@froyo-np froyo-np Jan 10, 2025

Choose a reason for hiding this comment

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

what happened here? why do these all have [' '] wrappings?
whats wrong with structureAnnotation: {...}

onComplete?: (imgSize: vec2) => void;
}

function decodeDzi(s: string, url: string): DziImage | undefined {
Copy link
Collaborator

@froyo-np froyo-np Jan 10, 2025

Choose a reason for hiding this comment

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

this is is a nice-ish function that shows how to decode the dzi xml/json metadata "format".

I think you should add some documentation, and maybe clean up or explain the url.split('.dzi') stuff

callback(images);
}

export function useDziImages({ imageUrls, onComplete }: useDziImageProps) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

useDziImages and getImages are unused, stateful, clunky - lets remove them?

const size = sizeInUnits('xy', v.multiscales[0].axes, v.multiscales[0].datasets[0]);
if (size) {
console.log(size);
setView(Box2D.create([0, 0], [size[0], size[1]]));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
setView(Box2D.create([0, 0], [size[0], size[1]]));
setView(Box2D.create([0, 0], size));

* @param {string} name - name will show up in console log
* @param {Object} props - entire props object of the component being tested
*/
export function useWhyDidYouUpdate<P extends object>(name: string, props: P) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

remove this file?

import type { ZarrSliceGridConfig } from '~/data-sources/ome-zarr/slice-grid';
import type { ScatterplotGridConfig, ScatterPlotGridSlideConfig } from '~/data-sources/scatterplot/dynamic-grid';

const slide32 = 'MQ1B9QBZFIPXQO6PETJ';
Copy link
Collaborator

Choose a reason for hiding this comment

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

ok this whole file is unused - it was nice that it was pulled out of the code for the layers example logic, but the types got a bit messed up.
either fix the types and use it, or delete it and keep the actual values on line 745 of src/layers.ts

@@ -61,6 +61,7 @@
"lodash": "^4.17.21",
"react": "^18.3.0",
"react-dom": "^18.3.0",
"react-router": "^7.0.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

are a handful of static, independant pages worth pulling in this dependency?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suppose it does let us delete the kinda gross repetetative {demo-name.html} files....

Copy link
Collaborator

Choose a reason for hiding this comment

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

lets keep it, seems fine

return (
<canvas
id={id}
ref={cnvs}
onWheel={wheel}
Copy link
Collaborator

Choose a reason for hiding this comment

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

so this seemed to work before - why copy paste the long-winded version from bkp-client?

});
}, []);

const zoom = (e: WheelEvent) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I thought part of the improvements we were going to add was around the cameras - currently the dzi demo cant pan, only zoom. Perhaps we could consolidate their zoom and pan handlers and have both working on both?

Copy link
Collaborator

@froyo-np froyo-np left a comment

Choose a reason for hiding this comment

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

Cleanup please, consider camera event handing consolidation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants