48 make sidebar more intuitive #52
Conversation
Write out name of each measurement tool and move measurements and saved positions above appearance
…group-4/MolloyExplorer into 48-make-sidebar-more-intuitive
…group-4/MolloyExplorer into 48-make-sidebar-more-intuitive
…bar-more-intuitive
WalkthroughAdds a Show/Hide labels toggle to the annotation panel, updates annotation and measurement panel DOM/CSS (new layout, button and tool-label wrappers), moves the measurements panel anchor, updates filter legend markup and styling, and removes the filters menu element during GUI init. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Panel as Annotation Panel
participant DOM as potree_annotation_container
User->>Panel: Click "Show" button
Panel->>Panel: set Show active, Hide inactive
Panel->>DOM: DOM.style.display = '' /* reveal labels */
User->>Panel: Click "Hide" button
Panel->>Panel: set Hide active, Show inactive
Panel->>DOM: DOM.style.display = 'none' /* hide labels */
sequenceDiagram
participant Init as Measurements init
participant Sidebar as Sidebar DOM (#menu_appearance)
participant Script as Wrapper creation
Init->>Sidebar: locate `#menu_appearance` (new anchor)
Script->>Sidebar: for each known tool icon -> create `.tool-with-label`
Script->>Sidebar: move icon into wrapper, append `.tool-label`
Script->>Script: bind click on wrapper to trigger original icon action
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Comment |
![coderabbit[bot]](https://avatars.git.ntnu.no/u/70329?s=60){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/MeasurementControl/measurementsPanel.js (1)
32-40: Fix critical null reference bug at line 668 and harden jQuery/accessibility.Three issues confirmed:
Critical null reference: Line 668 calls
existingTools.querySelectorAll()outside the null-check block (line 640), causing TypeError ifgetElementById('tools')returns null.Unguarded jQuery: Lines 41, 43, 47 use
$(menu)without checking if$is defined; line 48 shows guard is used elsewhere, indicating inconsistent pattern. Risk: ReferenceError if jQuery not loaded globally.Missing accessibility: Wrapper divs (line 679) lack
role="button",tabindex,aria-label, and keyboard handler—causing screenreader/keyboard navigation failures.Icon filename map verified complete: all 11 assets exist in
public/build/potree/resources/icons/.Apply the suggested diff to:
- Guard jQuery with
window.jQuery || window.$at lines 41–55- Null-guard
existingTools.querySelectorAll()at lines 668–690- Add
role,tabindex,aria-label, andkeydownlistener to wrapper divs
🧹 Nitpick comments (3)
src/potreeViewer.js (1)
260-303: Prototype monkey‑patching: limit blast radius.Temporarily overriding Element.prototype.scrollIntoView and HTMLElement.prototype.focus can cause surprising behavior elsewhere. Consider a scoped workaround (e.g., CSS overscroll-behavior, guarding scroll in a single container, or temporarily setting scrollTop on the specific accordion container) instead of patching prototypes globally. If you keep this, add a unique flag to avoid nested patches and ensure restore on all paths.
src/MeasurementControl/measurementsPanel.css (1)
275-301: Add focus state for keyboard accessibility on clickable wrappers.Wrappers are clickable; give them a visible focus style for keyboard users.
Apply:
.tool-with-label:hover { box-shadow: 0 0 5px #fff8; } +.tool-with-label:focus-visible { + outline: 2px solid #7ba8ff; + outline-offset: 2px; + box-shadow: 0 0 0 2px #7ba8ff44; +} + .tool-with-label:hover img { filter: brightness(1.7); }src/AnnotationControl/annotationPanel.css (1)
400-400: Hardcoded black border inconsistent with theme.Line 400 uses
border: 1px solid black;which appears inconsistent with the rest of the color scheme (e.g., borders elsewhere use#555,#404a50, etc.). Consider using a theme-consistent color variable or hex value to maintain visual consistency..toggle-group { display: flex; width: 265px; - border: 1px solid black; + border: 1px solid #555; border-radius: 4px; overflow: hidden; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/AcceptedFiltering/threePanels.css(2 hunks)src/AcceptedFiltering/threePanels.js(1 hunks)src/AnnotationControl/annotationPanel.css(2 hunks)src/AnnotationControl/annotationPanel.js(2 hunks)src/MeasurementControl/measurementsPanel.css(1 hunks)src/MeasurementControl/measurementsPanel.js(2 hunks)src/potreeViewer.js(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/potreeViewer.js (1)
src/AcceptedFiltering/threePanels.js (1)
$(199-199)
🔇 Additional comments (2)
src/AcceptedFiltering/threePanels.js (1)
302-309: LGTM: clearer legend copy.Explicit “Accepted points displayed as:” and “Not accepted points displayed as:” improves clarity and aligns with pills.
src/AnnotationControl/annotationPanel.css (1)
349-354: Class naming change is consistent and complete—no breaking selector updates needed.Verification confirms the class name change from
.pv-menu-list.annotations-panelto.pv-menu-list_annotations-panelis fully applied across the codebase with no orphaned references:
- CSS definition (line 349) matches JavaScript references (lines 34, 74)
- No old selector patterns exist anywhere in the repository
- All three references use the new underscore-based naming consistently
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/AnnotationControl/annotationPanel.css (1)
378-413: Adjust toggle UI border color and consider width flexibility.Two minor issues with the new label toggle styling:
Line 391 — Inconsistent border color: The
.toggle-groupusesborder: 1px solid black;which is inconsistent with other borders in the file (e.g., #404a50, #555, #1px solid #555). Use a theme-appropriate gray instead.Line 390 — Hardcoded width: The
width: 265px;is rigid and may cause layout issues on smaller viewports or if button labels change. Consider using a max-width with flexible sizing or ensuring the parent container allows responsive width adjustments.Additionally, verify that the button color scheme (
#a7a9aa,#3d3c3c,#c7c9ca) aligns with the overall design intent for this control.Apply this diff to fix the border color consistency:
.toggle-group { display: flex; width: 265px; - border: 1px solid black; + border: 1px solid #555; border-radius: 4px; overflow: hidden; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/AnnotationControl/annotationPanel.css(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-29T13:42:17.971Z
Learnt from: kleinc
PR: TDT4290-group-4/MolloyExplorer#52
File: src/AnnotationControl/annotationPanel.css:30-31
Timestamp: 2025-10-29T13:42:17.971Z
Learning: In src/AnnotationControl/annotationPanel.css, the .annotation-edit-textarea element intentionally has both background and color set to #636262, making the text invisible. This is the intended design per kleinc's preference.
Applied to files:
src/AnnotationControl/annotationPanel.css
📚 Learning: 2025-10-29T13:45:51.930Z
Learnt from: kleinc
PR: TDT4290-group-4/MolloyExplorer#52
File: src/AnnotationControl/annotationPanel.js:33-35
Timestamp: 2025-10-29T13:45:51.930Z
Learning: In src/AnnotationControl/annotationPanel.js, the panel className should be set to 'pv-menu-list_annotations-panel' only, without including the base 'pv-menu-list' class. This is intentional and correct.
Applied to files:
src/AnnotationControl/annotationPanel.css
🔇 Additional comments (2)
src/AnnotationControl/annotationPanel.css (2)
349-354: Verify annotation row width behavior with flex centering.The parent container uses
align-items: center;which horizontally centers all flex children. While this is appropriate for the fixed-width toggle button group and the 80%-width add button, ensure that annotation rows (.annotation-row) display at their intended width and don't collapse due to center alignment.
357-376: Button styling looks good.The updated add button styling integrates well with the new flex-based panel layout. The color, sizing, and transition effects are consistent with the overall design. The
display: block;behavior works correctly within the parent's flex centering context.
Add more padding Co-authored-by: Adrian Haabpiht Solberg <adriahso@stud.ntnu.no>
… 48-make-sidebar-more-intuitive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/potreeViewer.js (1)
58-61: Consider removing these lines if panels are closed by default.As suggested in previous feedback, if the panels are already closed by default, these lines may be unnecessary. Removing them would simplify the code and avoid the unguarded jQuery usage.
- $('#menu_appearance').next().hide() - $('#menu_tools').next().hide() - $('#menu_scene').next().hide() - $('#menu_filters').remove()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Filter/filter.css(2 hunks)src/Filter/filter.js(1 hunks)src/potreeViewer.js(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-10-29T13:42:17.971Z
Learnt from: kleinc
Repo: TDT4290-group-4/MolloyExplorer PR: 52
File: src/AnnotationControl/annotationPanel.css:30-31
Timestamp: 2025-10-29T13:42:17.971Z
Learning: In src/AnnotationControl/annotationPanel.css, the .annotation-edit-textarea element intentionally has both background and color set to #636262, making the text invisible. This is the intended design per kleinc's preference.
Applied to files:
src/Filter/filter.csssrc/potreeViewer.js
📚 Learning: 2025-10-29T13:45:51.930Z
Learnt from: kleinc
Repo: TDT4290-group-4/MolloyExplorer PR: 52
File: src/AnnotationControl/annotationPanel.js:33-35
Timestamp: 2025-10-29T13:45:51.930Z
Learning: In src/AnnotationControl/annotationPanel.js, the panel className should be set to 'pv-menu-list_annotations-panel' only, without including the base 'pv-menu-list' class. This is intentional and correct.
Applied to files:
src/potreeViewer.js
🧬 Code graph analysis (1)
src/potreeViewer.js (1)
src/Filter/filter.js (1)
$(202-202)
🔇 Additional comments (2)
src/Filter/filter.js (1)
304-313: LGTM! Clear legend labels improve usability.The descriptive text before each color indicator makes the legend more intuitive and accessible, aligning well with the PR objective to improve filter clarity.
src/Filter/filter.css (1)
1-134: LGTM! CSS updates enhance visual consistency.The styling changes improve the filter UI with centered button alignment, pill-shaped legend indicators, consistent text colors, and proper spacing. These updates support the PR's goal of making the sidebar more intuitive and visually polished.
…ble when pressing "add a location"
…utton more intuitive
Issue number
Closes #48
Description
Make the sidebar more intuitive, so that it is easy to understand what each tool is and where to find them:
Testing steps
Screenshots (optional)
Summary by CodeRabbit
New Features
Style
UI Changes