VV - UI + Styles #6573

kieftrav · 2024-12-17T15:27:03Z

Package

lib-subject-viewers

Describe your changes

Add 4, 8, 16, 32, and 64 example to Storybook
Add Histogram range slider
Fix scroll bug
Inline OrbitControls import
Incorporate Figma Styles

How to Review

Run lib-subject-viewers Storybook locally
Inspect styles in the deployed project-branch
Alternatively, you can run the local app-project

Please Review

Are layout and color styles implemented correctly?
- Test both Desktop and Mobile in app-project or on the branch deploy
Does resizing the window still enable full annotating?
Planes
- Does the toggle to expand/collapse work?
- Does the "forward 10" move forward 10 frames?
- Does holding the "forward 10" move forward at an interval?
- Does letting go of the "forward 10" stop moving forward at an interval?
- Does adjusting the slider work?
- Does clicking anywhere on the cube create+add to an the annotation?
- Does holding shift+click on the cube create a new annotation?
Cube
- Can you rotate the cube?
- Can you see annotations?
- Does clicking anywhere on the cube create+add to an the annotation?
- Does holding shift+click on the cube create a new annotation?
Histogram
- Do both the plane and the cube remove pixels/voxels when the histogram range is narrowed?
- Does the Histogram drawing re-normalize when adjusting the sliders?

Design Review Notes (from meeting with Sean)

Hide coordinate when collapsed
Keep coordinate size smaller when expanded
Start page with x slice open
20px padding on the cube
Remove rounded corners on cube
Remove stroke from dark mode
Cut out the bottom black from the histogram
Click and hold to “loop through”
When the histogram and axes touch is the min size for the cube
- When it gets to that size, then move to stacked layout
When threshold is moved, sometimes the annotations are not redrawn and remain hidden until the frame is changed

Future PR / Not Implemented

Ability to delete an Annotation in the cube
Ability to "undo" in the Toolbar
Get the outline to travel with the frame (math is complicated)

Checklist

PR Creator - Please cater the checklist to fit the review needed for your code changes.
PR Reviewer - Use the checklist during your review. Each point should be checkmarked or discussed before PR approval.

General

Tests are passing locally and on Github
Documentation is up to date and changelog has been updated if appropriate
You can yarn panic && yarn bootstrap or docker-compose up --build and FEM works as expected
FEM works in all major desktop browsers: Firefox, Chrome, Edge, Safari (Use Browserstack account as needed)
FEM works in a mobile browser

General UX

Example Staging Project: i-fancy-cats

All pages of a FEM project load: Home Page, Classify Page, and About Pages
Can submit a classification
Can sign-in and sign-out
The component is accessible
- Can be used with a screen reader BBC guide to VoiceOver
- Can be used from the keyboard WebAIM guide to keyboard testing
- It is passing accessibility checks in the storybook

coveralls · 2024-12-17T15:34:42Z

coverage: 75.607% (-1.7%) from 77.338%
when pulling 5232070 on vv-styles
into 51f064e on master.

- Add 4, 8, 16, 32, and 64 example to Storybook - Add Histogram range slider - Add params to overall Viewer - Fix scroll bug - Inline OrbitControls import - Incorporate Figma Styles

kieftrav · 2025-01-04T15:27:33Z

@seanmiller26 - I have incorporated all of your suggestions minus the outline for the frames within the Cube... This proved more mathematically difficult so I've punted for when I'm back. I also took the liberty of re-adding the rounded corners in dark mode on mobile because it follows the design intention more consistently. Easy to change if you disagree!

goplayoutside3

I reviewed by running lib-subject-viewer's Storybook and testing the workflow of your VV test project and the NeuroTracerBrain test project. I've left a few comments/questions, but otherwise skimmed the lib-subject-viewer code because 2k lines of changes is a lot for an in-depth review.

The steps in How To Review work as expected, but I have a few additional UI questions:

Should the interaction to expand / collapse include clicking the text label? For example, as a user I expected to be able to click on the word "Expand" or the carat icon to expand the plane, but for now only the carat can be interacted with.
Have you noted the accessibility violations in Storybook? For instance, the Slider in particular is missing accessible labels. And in general, the Cube area doesn't have aria to announce the contents to a screen reader (for example, the bar chart for user stats has an aria-label to communicate the description of the bar chart). These points don't have to be resolved in this PR, but definitely should be documented todos, maybe in the project board?
The test project UI includes zoom buttons and rotate button in the image toolbar. What are the plans for those and is the intended UX documented in the project board?

And lastly, this is potentially already behavior you're aware of, but there's a console error generated by Cube.js TypeError: Cannot set properties of undefined (setting 'x'). I can reliably produce the error by running NeuroTracerBrain locally in dev mode. In dev mode, it's a console error, but in production mode there's no error. With this test project in particular, it also takes an unexpectedly long time for annotations to show up on the Cube and Plane after a user click (about 5 - 10 seconds). The delay could be unrelated to the console error, but wanted to document a detailed UX during review so performance issues and errors can be added to the project board if not addressed in this PR.

goplayoutside3 · 2025-01-16T17:41:04Z

...s/Classifier/components/SubjectViewer/components/VolumetricViewer/VolumetricViewerWrapper.js

-      subject={subject}
-    />
+  const config = {
+    annotations: [],


annotations is being defined here as a prop, but not used in the React component it's passed to 🤔 Did you mean to? Looking at the VolumetricViewer in lib-subject-viewers, the component does not expect to receive annotations as a prop.

goplayoutside3 · 2025-01-16T18:13:17Z

packages/lib-subject-viewers/src/VolumetricViewer/README.md

@@ -4,3 +4,5 @@ This directory holds all the relevant code for rendering the VolumetricViewer. T

 - `VolumetricViewerComponent` - a React component for the VolumetricViewer
 - `VolumetricViewerData` - a function that returns the data with instantiated models along with the React Component
+
+NOTE: Webpack has trouble importing from a node_module package from dependent package chains (`lib-subject-viewers` => `lib-classifier` => `app-project`), therefore we have inlined `OrbitControls.js` file so that it works without Webpack issues. When updating the `three` dependency in `package.json` make sure to copy the file from `node_modules/three/examples/jsm/controls/OrbitControls.js` to `lib-subject-viewers/src/VolumetricViewer/helpers/OrbitControls.js`.


Suggested change in wording because FEM can use node_module resources lib => lib-classifier => app-project, and an example is whenever the repo requires lib-react-components => lib-classifier => app-project. However, in this case OrbitControls is a ES Module, and FEM is not pure ESM (hence the cjs export scripts in all package.json), so trying import { OrbitControls } from 'three/examples/jsm/controls/OrbitControls' throws the error ESM packages (three/examples/jsm/controls/OrbitControls) need to be imported in lib-subject-viewers/dist/cjs/VolumetricViewer/components/Cube.js.

Suggested change

NOTE: Webpack has trouble importing from a node_module package from dependent package chains (`lib-subject-viewers` => `lib-classifier` => `app-project`), therefore we have inlined `OrbitControls.js` file so that it works without Webpack issues. When updating the `three` dependency in `package.json` make sure to copy the file from `node_modules/three/examples/jsm/controls/OrbitControls.js` to `lib-subject-viewers/src/VolumetricViewer/helpers/OrbitControls.js`.

NOTE: OrbitControls are typically imported as `import { OrbitControls } from 'three/examples/jsm/controls/OrbitControls'` but that syntax throws an error due to FEM not being pure ESM, therefore we have inlined `OrbitControls.js` file so that it works without bundling issues. When updating the `three` dependency in `package.json` make sure to copy the file from `node_modules/three/examples/jsm/controls/OrbitControls.js` to `lib-subject-viewers/src/VolumetricViewer/helpers/OrbitControls.js`.

goplayoutside3 · 2025-01-16T19:18:02Z

packages/lib-subject-viewers/src/VolumetricViewer/components/Slider.js

+/* RAW SVG FOR SLIDER | Needs to be URL encoded to view
+const SVGSlider = `<svg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 40 32'>
+  <rect width='100%' height='100%' rx='8' ry='8' style='fill: rgb(0,93,105);' />
+  <path d='M16 10 l-7 6 l7 6;' style='stroke:rgb(173, 221, 224); stroke-width:2; fill: none;'/>
+  <path d='M24 10 l7 6 l-7 6;' style='stroke:rgb(173, 221, 224); stroke-width:2; fill: none;'/>
+</svg>`
+*/


I noticed this is commented-out, did you want to keep it in the Slider.js code?

goplayoutside3 · 2025-01-16T19:43:30Z

packages/lib-subject-viewers/src/VolumetricViewer/models/ModelViewer.js

@@ -14,7 +14,7 @@ export const ModelViewer = () => {
    planesAbsoluteSets: [[], [], []],
    planeFrameActive: [0, 0, 0],
    points: [],
-    threshold: { min: 0, max: 255 },
+    threshold: { min: 5, max: 255 },  // min of 5 cuts out missing data noise


Out of curiosity, will uploaded subjects possibly have missing data?

kieftrav marked this pull request as draft December 17, 2024 17:27

kieftrav force-pushed the vv-styles branch 4 times, most recently from 69b6604 to 069b614 Compare December 19, 2024 12:42

kieftrav changed the title ~~VV - Styles~~ VV - UI + Styles Dec 19, 2024

kieftrav marked this pull request as ready for review December 19, 2024 13:02

kieftrav requested a review from seanmiller26 December 19, 2024 13:03

kieftrav requested a review from goplayoutside3 January 4, 2025 13:42

kieftrav added 3 commits January 4, 2025 07:43

VV: UI + Design

3fd075f

- Add 4, 8, 16, 32, and 64 example to Storybook - Add Histogram range slider - Add params to overall Viewer - Fix scroll bug - Inline OrbitControls import - Incorporate Figma Styles

URL encode SVG

110e92e

Implement design review styles.

bb09741

kieftrav force-pushed the vv-styles branch from a4fe086 to bb09741 Compare January 4, 2025 13:43

Fix tests

5232070

goplayoutside3 reviewed Jan 16, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VV - UI + Styles #6573

VV - UI + Styles #6573

kieftrav commented Dec 17, 2024 •

edited

Loading

coveralls commented Dec 17, 2024 •

edited

Loading

kieftrav commented Jan 4, 2025

goplayoutside3 left a comment

goplayoutside3 Jan 16, 2025

goplayoutside3 Jan 16, 2025

goplayoutside3 Jan 16, 2025

goplayoutside3 Jan 16, 2025

VV - UI + Styles #6573

Are you sure you want to change the base?

VV - UI + Styles #6573

Conversation

kieftrav commented Dec 17, 2024 • edited Loading

Package

Describe your changes

How to Review

Checklist

General

General UX

coveralls commented Dec 17, 2024 • edited Loading

kieftrav commented Jan 4, 2025

goplayoutside3 left a comment

Choose a reason for hiding this comment

goplayoutside3 Jan 16, 2025

Choose a reason for hiding this comment

goplayoutside3 Jan 16, 2025

Choose a reason for hiding this comment

goplayoutside3 Jan 16, 2025

Choose a reason for hiding this comment

goplayoutside3 Jan 16, 2025

Choose a reason for hiding this comment

kieftrav commented Dec 17, 2024 •

edited

Loading

coveralls commented Dec 17, 2024 •

edited

Loading