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

VV: Add VolumetricViewer to lib-subject-viewers #6349

Merged
merged 5 commits into from
Oct 14, 2024
Merged

Conversation

kieftrav
Copy link
Contributor

@kieftrav kieftrav commented Sep 30, 2024

Package

  • lib-subject-viewers

Describe your changes

  • Add "data-testid" to all DOM elements we want to test for existence
  • Add PropTypes to all Components based on linter output
  • Add shims for Canvas, WebGL, and requestAnimationFrame for Cube rendering and testing
  • Restructure VolumetricViewer directory to have all components, css, data, helpers, models, and tests in respective directories
  • Specs for AlgorithmAStar
  • Specs for ModelAnnotation
  • Specs for ModelTool
  • Specs for ModelViewer
  • Specs for pointColor
  • Specs for SortedSet
  • Specs for VolumetricViewer that simply tests for existence in the DOM
  • Write skeleton of a README

How to Review

  • Load up the lib-subject-viewers in Storybook. Should load the 3 Plane frames on the left and the 1 3D Cube on the right. No spec testing of Components beyond "does it load" in favor of Storybook testing for UI usage & needing to incorporate Figma
  • All specs should pass. Please review for missing behavior
  • Please advise on where the shim for server testing that's currently in Cube.js should go
  • Please advise on if anything more should be added to the README at this time
  • The ModelAnnotations has a half-finished implementation of History. I plan on finishing that in a follow up PR

** Note: The last 2 commits were to get the GH ci:test to pass. I couldn't figure out how to get the gl package to build in that context. Running tests locally without the last 2 PR's removing that works locally. I'd appreciate guidance on if we want to spend more time getting gl to work for the ci testrunner... Because the main point of it is to make sure the components load. If we're relying on Storybook for these components + unit testing the underlying Models, I think we'd be all right.

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

New Feature

  • The PR creator has listed user actions to use when testing the new feature
  • Unit tests are included for the new feature
  • A storybook story has been created or updated
    • Example of SlideTutorial component with docgen comments, and its story

@kieftrav kieftrav marked this pull request as draft September 30, 2024 21:50
@kieftrav kieftrav self-assigned this Sep 30, 2024
@kieftrav kieftrav force-pushed the vv-in-subject-viewers branch 2 times, most recently from 233960f to 2898989 Compare October 1, 2024 15:23
@coveralls
Copy link

coveralls commented Oct 1, 2024

Coverage Status

coverage: 77.791% (-0.8%) from 78.542%
when pulling a0981f0 on vv-in-subject-viewers
into 4062f39 on master.

@kieftrav kieftrav marked this pull request as ready for review October 1, 2024 17:41
@kieftrav kieftrav changed the title DRAFT: Add VolumetricViewer to lib-subject-viewers Add VolumetricViewer to lib-subject-viewers Oct 1, 2024
@kieftrav kieftrav force-pushed the vv-in-subject-viewers branch 2 times, most recently from bf34584 to 269d0ba Compare October 4, 2024 15:55
Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

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

Code read mostly looks good! Per our conversation earlier today this PR copies some of the viewer code from brainsweeper.zooniverse.org into lib-subject-viewers with plans to break up the code into smaller pieces for later integration into the classifier.

However, I'm now having trouble running this branch locally, so could you check a couple of things for me?

  1. canvas and watch are added to the package.json, but I see no changes to yarn.lock. Either double check that you installed these third party libraries via yarn add [package], or simply run yarn at the front-end-monorepo root. Because this is a monorepo workspace, installation of new third party libraries will result in changes to the yarn.lock file.
  2. I noted in Slack that Node released a new version of lts/iron today and that it actually affected my ability to bootstrap FEM locally. I can bootstrap the master branch, but when I try to bootstrap this PR branch locally, I immediately get errors about canvas, so that could be caused by my first point above, but also please check which version of Node you're using locally. For consistency, it'll be easier to debug if we're both using the same version of Node and the same version as the CI action.

@kieftrav
Copy link
Contributor Author

kieftrav commented Oct 8, 2024

@goplayoutside3 - I had originally excluded the yarn.lock changes as per a previous conversation we had had. Just added it.

- Add "data-testid" to all DOM elements we want to test for existence
- Add PropTypes to all Components based on linter output
- Add shims for Canvas, WebGL, and requestAnimationFrame for Cube rendering and testing
- Restructure VolumetricViewer directory to have all components, css, data, helpers, models, and tests in respective directories
- Specs for AlgorithmAStar
- Specs for ModelAnnotation
- Specs for ModelTool
- Specs for ModelViewer
- Specs for pointColor
- Specs for SortedSet
- Specs for VolumetricViewer that simply tests for existence in the DOM
- Write skeleton of a README
Copy link
Contributor

@goplayoutside3 goplayoutside3 left a comment

Choose a reason for hiding this comment

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

I'm using a Macbook with M1 chip and installing canvas locally for the VolumetricViewer requires extra setup. Instructions can be found here Automattic/node-canvas#1511.

@kieftrav I opened #6385 to document troubleshooting the new viewer. Please merge it into this PR and you're good to go 👍

I had originally excluded the yarn.lock changes as per a previous conversation we had had. Just added it.

Changes to the yarn.lock file should always be included in PRs. For instance, when a new third-party library is added to FEM, the monorepo tracks versioning of those libraries in yarn.lock. There may have been some misunderstanding in past conversations, but yarn.lock is crucial to the monorepo workspace.

@github-actions github-actions bot added the approved This PR is approved for merging label Oct 14, 2024
@kieftrav kieftrav changed the title Add VolumetricViewer to lib-subject-viewers VV: Add VolumetricViewer to lib-subject-viewers Oct 14, 2024
@kieftrav kieftrav merged commit c0c1959 into master Oct 14, 2024
7 checks passed
@kieftrav kieftrav deleted the vv-in-subject-viewers branch October 14, 2024 20:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved This PR is approved for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants