feat(slice scrollbar): Integrate ViewportSliceProgressScrollbar with customizations and docs - #5960
Conversation
… what is done for white labeling.
…t-scrollbar' into feature/OHIF-2572-integrate-smart-scrollbar
…/github.com/jbocce/Viewers into feature/OHIF-2572-integrate-smart-scrollbar
…Indicator may take advantage of it.
…t-scrollbar' into feature/OHIF-2572-integrate-smart-scrollbar
…ntegrate-smart-scrollbar
✅ Deploy Preview for ohif-dev canceled.
|
… to match the default value in the customization service and the documentation.
…ntegrate-smart-scrollbar
… into a subdirectory of files. Update the loading className of the viewed fill to match the original design.
…rnerstone extension.
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
| { | ||
| id: 'viewportScrollbar.loadedBatchIntervalMs', | ||
| description: | ||
| 'Batch interval in milliseconds for loaded/cached slice updates in full progress mode.', |
There was a problem hiding this comment.
I'm not sure what this configuration is, could you explain it?
There was a problem hiding this comment.
@sedghi I updated the description. Let me know what you think. Basically there are so many cached updates that I wanted a wait to group/coalesce them together.
There was a problem hiding this comment.
Switching to full screen for annotation tests seems like it would reduce accuracy. Can you just leave it as the viewport image?
There was a problem hiding this comment.
Hmmm. So the good news is I am no longer changing this screenshot. However, the original switch was from full app screenshot to the viewport grid which reduces false negatives due to text differences. So to be honest I am not sure what you were referring to. But regardless it is moot point now. That said I did change another screenshot... see https://github.com/OHIF/Viewers/pull/5960/changes#diff-9fe49be77e592887412015f704e55db26f3cf0c1f31e1aff8b635eb65a969dc6 and that is definitely what we want. No need to compare toolbars and side panels too.
There was a problem hiding this comment.
I am also very happy about a change I made that allows the tests to inject different config defaults for themselves seamlessly. This came about because I wanted to NOT include any of the progress info (i.e. viewed, cached, loading, etc.) for the new scrollbar in the tests.
…ator because that is the focus of the test.
…entation for CAMERA_MODIFIED events.
Context
Special thanks to @dan-rukas for all his help on this feature.
Special thanks to all the collaborators of #4340 that helped shape and direct this PR.
Integrates the new progress-style viewport scrollbar (
ViewportSliceProgressScrollbar) withviewportScrollbar.*customization (variant, fills, endpoints, loading pattern, timing/batching, custom indicator). AddsSmartScrollbarupdates (includingrenderIndicator(React), layout context withisDragging, fill defaults),useByteArraybatching refinements, andViewedImagesServicefor viewed-slice tracking. Aligns Cornerstone viewport overlays / scrollbar placement with the new UI. Expands docs (Viewport Scrollbar customization page,ViewedImagesService, screenshots, sampleCustomizations /TableGeneratorhelpers).Changes & Results
ViewportSliceProgressScrollbar.tsx;viewportScrollbarCustomization.tsx; overlay tweaks (CornerstoneOverlays,CustomizableViewportOverlay).SmartScrollbar.tsx, indicator/fill/config,useByteArray.ts, exports;ViewportActionCorners/ViewportOverlayalignment.ViewedImagesServiceplus types/service registration.appInit.jswires the new service (small change).ViewportScrollbar.md,ViewedImagesService.md, services index links;sampleCustomizations.tsxupdates (captions,configurationIntro, viewport rows); 7 new PNGs underplatform/docs/docs/assets/img/.platform/ui/tailwind.config.jstoken tweak for scrollbar alignment.Testing
Run the deploy demo.
Stack viewports and acquisition-plane volume viewports should show the "full mode" new slice progress scrollbar - with the cached and viewed fills. So in MPR typically the acquisition viewport will show the full mode scrollbar and the two others will show the new slice progress scrollbar with no fills, endpoints, etc.
On a local checkout of the source branch of this PR, try the various configurations...
viewportScrollbar.variantprogress(ViewportSliceProgressScrollbar) orlegacy(ViewportImageScrollbar).progressviewportScrollbar.showLoadedEndpointstrueviewportScrollbar.showLoadedFilltrueviewportScrollbar.showViewedFilltrueviewportScrollbar.showLoadingPatterntrueviewportScrollbar.viewedDwellMs0marks immediately.0viewportScrollbar.loadedBatchIntervalMs200viewportScrollbar.indicatortotalWidth×totalHeight, border included) andrenderIndicatorcallback (receivesReact). IftotalWidth,totalHeight, andrenderIndicatorare not all valid, the default pill indicator is used.{}Here is a temporary link to the docs https://adorable-liger-3c240b.netlify.app/platform/services/customization-service/ViewportScrollbar. Don't expect this to stay up for long, but is provided as a convenience and for testing purposes.
Checklist
PR
semantic-release format and guidelines.
Code
etc.)
Public Documentation Updates
additions or removals.
Tested Environment
System:
OS: Windows 11 10.0.26200
CPU: (20) x64 12th Gen Intel(R) Core(TM) i7-12700H
Memory: 6.84 GB / 31.68 GB
Binaries:
Node: 20.9.0 - C:\Users\joebo\AppData\Local\fnm_multishells\51892_1775135638167\node.EXE
Yarn: 1.22.22 - C:\Program Files (x86)\Yarn\bin\yarn.CMD
npm: 10.1.0 - C:\Users\joebo\AppData\Local\fnm_multishells\51892_1775135638167\npm.CMD
bun: 1.2.23 - C:\Users\joebo.bun\bin\bun.EXE
Browsers:
Chrome: 146.0.7680.178
Edge: Chromium (146.0.3856.84)
Internet Explorer: 11.0.26100.7309
Greptile Summary
This PR integrates
ViewportSliceProgressScrollbarinto the Cornerstone extension, backed by a newViewedDataService, refactoredSmartScrollbarcomponents (custom indicator config,isDraggingcontext, corrected fill-strip pointer mapping), and updateduseByteArraybatching. Docs, E2E test infrastructure, and overlay alignment are also included. Previously flagged issues —scrollbarHeightexcess prop,setIntervalsemantics,loadedBatchIntervalMsfallback mismatch, deepPubSubServiceimport, and viewed-fillloadingClassName— are all addressed in this revision.Confidence Score: 4/5
PR is safe to merge; the one remaining concern is a minor test-utility design issue, not a runtime bug.
All previously flagged P0/P1 issues have been resolved. The only new finding is a P2 design flaw in addOHIFConfiguration where a caller-supplied customizationService key silently overwrites the baseline E2E scrollbar suppression, which could cause future screenshot flakiness but does not affect any current test in this PR.
tests/utils/OHIFConfiguration.ts — the object-spread merge order should be fixed before this utility is used with custom customizationService configs in other tests.
Important Files Changed
Comments Outside Diff (1)
platform/ui-next/src/components/SmartScrollbar/ViewportSliceProgressScrollbar.tsx, line 442-447 (link)SmartScrollbarFill'sloadingClassNamenow defaults to'bg-neutral/50'. The viewed-fill instance does not passloadingClassName, so it inherits the default. WhenisLoadingistrue,activeClassresolves to'bg-neutral/50'instead of the intended'bg-primary/35', making the viewed fill visually identical to the loaded fill's loading color.Pass the primary class as
loadingClassNameto keep the viewed fill visually distinct during loading:Prompt To Fix With AI
Prompt To Fix All With AI
Reviews (18): Last reviewed commit: "Add comment to useProgressScrollbarMode ..." | Re-trigger Greptile