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

feature: Loading from the landing page, viewer page routing #212

Merged

Conversation

ShrimpCryptid
Copy link
Contributor

@ShrimpCryptid ShrimpCryptid commented Apr 11, 2024

Estimated review size: medium, 30-40 minutes

Closes #188, the landing page implementation!

This change finishes implementing loading from the landing page and also sets up routing for the app.

These changes seem large, but the majority of the changes are just file moves and renames! I'll note what files you can compare between, and a summary of any changes on each file.

Changes:

  • Adds react-router-dom and routes for the landing page (at root URL, /) and viewer (/viewer).
  • The landing page can now redirect to the viewer when a Load button is clicked, and passes the app arguments as state through the navigation API.
  • Renamed the landing-page directory to website, since it now includes several website-related components.
  • Adds a new AppWrapper component, which wraps ImageViewerApp.
    • Handles retrieving app arguments from URL or from routing.
  • Moves the URL parsing logic to url_utils.tsx.

Videos (🔊)

Browser Router:

2024-04-12.11-38-40.mp4

Hash Router:

2024-04-12.11-40-15.mp4

Loading empty viewer:
image

Validation:

  1. Run the app locally. The default URL should open on the landing page.
  2. Click on the "Load" button, and it should open the volume in the /viewer page.
  3. Refreshing the page should still show the same volume.
  4. Go to http://localhost:9020/viewer?url=https://animatedcell-test-data.s3.us-west-2.amazonaws.com/20200323_F01_001/P13-C4.zarr . This should also open correctly in the browser.
  5. Go to http://localhost:9020?url=https://animatedcell-test-data.s3.us-west-2.amazonaws.com/20200323_F01_001/P13-C4.zarr . This will redirect to the /viewer page.

@ShrimpCryptid ShrimpCryptid requested a review from frasercl April 11, 2024 19:39
@ShrimpCryptid ShrimpCryptid self-assigned this Apr 11, 2024
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to website/LandingPage/content.ts. I updated the viewerSettings argument to enable channels, but otherwise everything else is the same.

Comment on lines -3 to -12
// TBD what URL parameters to include here
export type ViewerArgs = {
baseurl: string;
cellid: number;
cellPath: string;
fovPath: string;
fovDownloadHref: string;
cellDownloadHref: string;
viewerSettings: ViewerChannelSettings;
};
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was removed and replaced with the AppProps type that Cameron introduced in another PR.

Comment on lines -10 to -12
import { ImageViewerApp, RenderMode, ViewerChannelSettings, ViewMode } from "../src";
import FirebaseRequest, { DatasetMetaData } from "./firebase";
import { AppProps, GlobalViewerSettings } from "../src/aics-image-viewer/components/App/types";
Copy link
Contributor Author

Choose a reason for hiding this comment

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

All of the parsing logic below was moved to src/website/utils/url_utils.tsx.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This code was throwing an error due to the various refs being undefined on startup.

@@ -0,0 +1,54 @@
import React, { ReactElement } from "react";
Copy link
Contributor Author

@ShrimpCryptid ShrimpCryptid Apr 12, 2024

Choose a reason for hiding this comment

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

This file is copied from https://github.com/allen-cell-animated/nucmorph-colorizer/blob/main/src/routes/ErrorPage.tsx. The only change was editing the page content to give a more helpful error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved this file and edited the groups argument. Same as the deleted content.ts file.

@ShrimpCryptid ShrimpCryptid requested a review from toloudis April 12, 2024 17:37
@ShrimpCryptid ShrimpCryptid marked this pull request as ready for review April 12, 2024 18:45
@ShrimpCryptid ShrimpCryptid requested a review from a team as a code owner April 12, 2024 18:45
@ShrimpCryptid ShrimpCryptid linked an issue Apr 12, 2024 that may be closed by this pull request
…-styling

- Adds the banner video MP4, which autoplays when loaded.
  - Also added an accessibility feature, where the video playback is disabled if `prefers-reduced-motion` is set to true.
- Adds `Open Sans` via Google fonts. Existing fonts are still included to ensure compatibility with embedded use cases, but will be removed in the future.
- Adds the `StyleProvider` component, which will wrap the `LandingPage` and `App` in the future to provide a central location for CSS variables.
- Implements styling for the landing page.
@ShrimpCryptid ShrimpCryptid removed the request for review from a team April 15, 2024 21:28
src/website/types.ts Outdated Show resolved Hide resolved
// vars filled at build time using webpack DefinePlugin
console.log(`website-3d-cell-viewer ${WEBSITE3DCELLVIEWER_BUILD_ENVIRONMENT} build`);
console.log(`website-3d-cell-viewer Version ${WEBSITE3DCELLVIEWER_VERSION}`);
console.log(`volume-viewer Version ${VOLUMEVIEWER_VERSION}`);

export const VIEWER_3D_SETTINGS: ViewerChannelSettings = {
Copy link
Contributor

Choose a reason for hiding this comment

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

good to delete yes, but wondering how we can get test data like this into the viewer during dev. These settings are basically how we want the variance dataset to be presented in the viewer.

src/website/components/LandingPage/content.ts Outdated Show resolved Hide resolved
src/website/utils/url_utils.ts Outdated Show resolved Hide resolved
@ShrimpCryptid
Copy link
Contributor Author

good to delete yes, but wondering how we can get test data like this into the viewer during dev. These settings are basically how we want the variance dataset to be presented in the viewer.

Can't respond directly to this for some reason, but I did end up adding props for the AppWrapper component where this could be overridden. You could also edit LandingPage/content.ts to add a button for whatever test data you're working with, or include it via the URL?

@ShrimpCryptid ShrimpCryptid requested a review from toloudis April 17, 2024 16:47
- Removes the Merriweather Sans and Overpass font files and replaces them with Open Sans.
@ShrimpCryptid ShrimpCryptid marked this pull request as draft April 18, 2024 17:04
@ShrimpCryptid
Copy link
Contributor Author

ShrimpCryptid commented Apr 18, 2024

Leaving further refactor of project until #215.

@ShrimpCryptid ShrimpCryptid marked this pull request as ready for review April 18, 2024 18:05

import "antd/dist/antd.less";

// Components
import AppWrapper from "../website/components/AppWrapper";
import LandingPage from "../website/components/LandingPage";
import ErrorPage from "../website/components/ErrorPage";
Copy link
Contributor

Choose a reason for hiding this comment

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

Moving files to a separate website/ dir feels better. But doesn't it seem cleaner to put the contents of website under this public/ dir where index.tsx lives? I'm not sure if now is the time, but moving webpack config and the tsconfig that includes website files into the public app dir would help isolate things more clearly within this repo.

Copy link
Contributor

Choose a reason for hiding this comment

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

#215 contains the correct answer!

"compilerOptions": {
"module": "ES6"
"module": "ES6",
"outDir": "./dist"
Copy link
Contributor

Choose a reason for hiding this comment

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

Sanity check: To confirm that the separation is still good, run npm build and check the es/ directory contains none of the website modules.

import { ViewMode } from "../../src/aics-image-viewer/shared/enums";
import { ViewerChannelSettings } from "../../src/aics-image-viewer/shared/utils/viewerChannelSettings";

const paramKeys = ["mask", "ch", "luts", "colors", "url", "file", "dataset", "id", "view"];
Copy link
Contributor

Choose a reason for hiding this comment

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

an interesting todo is to add comments describing how these are parsed. In particular, the lut setting has some special shorthand encoding using p for percentile and m for median. For later..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will likely do this when #196 is implemented 👍

Copy link
Contributor

@frasercl frasercl left a comment

Choose a reason for hiding this comment

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

Looks good overall!

website/components/LandingPage/index.tsx Outdated Show resolved Hide resolved
public/index.tsx Outdated Show resolved Hide resolved
website/components/AppWrapper.tsx Outdated Show resolved Hide resolved
@ShrimpCryptid ShrimpCryptid merged commit d69c6f8 into feature/landing-page-styling Apr 23, 2024
3 checks passed
@ShrimpCryptid ShrimpCryptid deleted the feature/loading-from-landing-page branch April 23, 2024 16:37
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.

landing page
3 participants