fix: handle CLS issues in chart modal#2032
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change makes TrendsChart dynamically compute Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can scan for known vulnerabilities in your dependencies using OSV Scanner.OSV Scanner will automatically detect and report security vulnerabilities in your project's dependencies. No additional configuration is required. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Package/TrendsChart.vue (1)
1719-1726: Consider adding height to the transition for smoother expansion.Currently, only
opacityis transitioned (transition-[opacity]), so the container height snaps instantly frommax-h-0tomax-h-[220px]. If you want a smoother expand/collapse animation, you could transition both properties:- class="overflow-hidden transition-[opacity] duration-200 ease-out" + class="overflow-hidden transition-[max-height,opacity] duration-200 ease-out"If the instant height change is intentional to minimise perceived CLS, this is fine as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 906e8b30-3531-4b15-abc5-49dfb0829107
📒 Files selected for processing (1)
app/components/Package/TrendsChart.vue
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Package/TrendsChart.vue (1)
1698-1844:⚠️ Potential issue | 🟠 MajorMake the correction panel a proper disclosure.
Line 1711 only hides the panel visually, so the sliders on Lines 1726, 1741 and 1756 plus the checkbox on Line 1828 stay tabbable when the section is “closed”. The toggle on Line 1698 also does not expose its expanded state.
♿ Suggested fix
<button type="button" + :aria-expanded="showCorrectionControls" + aria-controls="trend-correction-controls" class="self-start flex items-center gap-1 text-2xs font-mono text-fg-subtle hover:text-fg transition-colors" `@click`="showCorrectionControls = !showCorrectionControls" > @@ <div + id="trend-correction-controls" + :aria-hidden="!showCorrectionControls" + :inert="!showCorrectionControls || undefined" class="overflow-hidden transition-[opacity] duration-200 ease-out" :class=" showCorrectionControls ? 'max-h-[220px] opacity-100' : 'max-h-0 opacity-0 pointer-events-none' " >
🧹 Nitpick comments (1)
app/components/Package/TrendsChart.vue (1)
1351-1356: Unify the chart-height maths in one place.
chartHeightnow uses950 / 600 / 494, while the wrapper on Lines 1861-1867 uses a separate567 / 491branch. Those values will drift over time and make this CLS fix brittle, because the outer container andVueUiXysizing can diverge silently.Also applies to: 1861-1867
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 63f7d82a-30ad-4bb3-a3ba-65fb3d163667
📒 Files selected for processing (1)
app/components/Package/TrendsChart.vue
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Package/TrendsChart.vue (1)
2170-2173: Scope.no-transitionstyles to this component rootThe selector is currently global and generic. Scoping it to
#trends-chartkeeps the transition guard local and avoids unintended styling collisions with other components using the same class name.Proposed refactor
-.no-transition line, -.no-transition circle { +#trends-chart .no-transition line, +#trends-chart .no-transition circle { transition: none !important; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3f9108c3-aaba-4626-adf5-6ca247467e32
📒 Files selected for processing (1)
app/components/Package/TrendsChart.vue
graphieros
requested review from
MatteoGabriele,
serhalp and
shuuji3
and removed request for
serhalp
There was a problem hiding this comment.
@graphieros Nice refinement ✨
I put two suggestions: simplifying with useTimeoutFn() and adding disabled prop.
| const isResizing = shallowRef(false) | ||
|
|
||
| const chartHeight = computed(() => { | ||
| if (isMobile.value) { | ||
| return 950 | ||
| } | ||
| return showCorrectionControls.value && props.inModal ? 494 : 600 | ||
| }) | ||
|
|
||
| const timeoutId = shallowRef<ReturnType<typeof setTimeout> | null>(null) | ||
|
|
||
| function pauseChartTransitions() { | ||
| if (timeoutId.value) { | ||
| clearTimeout(timeoutId.value) | ||
| } | ||
|
|
||
| isResizing.value = true | ||
|
|
||
| timeoutId.value = setTimeout(() => { | ||
| isResizing.value = false | ||
| timeoutId.value = null | ||
| }, 200) | ||
| } | ||
|
|
||
| onBeforeUnmount(() => { | ||
| if (timeoutId.value) { | ||
| clearTimeout(timeoutId.value) | ||
| } | ||
| }) |
There was a problem hiding this comment.
You can probably simplify timeout logic with useTimeoutFn() (ref. https://vueuse.org/shared/useTimeoutFn/). Calling start() resets the timer countdown from the beginning (playground), so this should provide the same behavior (not tested yet).
| const isResizing = shallowRef(false) | |
| const chartHeight = computed(() => { | |
| if (isMobile.value) { | |
| return 950 | |
| } | |
| return showCorrectionControls.value && props.inModal ? 494 : 600 | |
| }) | |
| const timeoutId = shallowRef<ReturnType<typeof setTimeout> | null>(null) | |
| function pauseChartTransitions() { | |
| if (timeoutId.value) { | |
| clearTimeout(timeoutId.value) | |
| } | |
| isResizing.value = true | |
| timeoutId.value = setTimeout(() => { | |
| isResizing.value = false | |
| timeoutId.value = null | |
| }, 200) | |
| } | |
| onBeforeUnmount(() => { | |
| if (timeoutId.value) { | |
| clearTimeout(timeoutId.value) | |
| } | |
| }) | |
| const isResizing = shallowRef(false) | |
| const chartHeight = computed(() => { | |
| if (isMobile.value) { | |
| return 950 | |
| } | |
| return showCorrectionControls.value && props.inModal ? 494 : 600 | |
| }) | |
| const { start } = useTimeoutFn(() => { | |
| isResizing.value = false | |
| }, 200, { immediate: false }) | |
| function pauseChartTransitions() { | |
| isResizing.value = true | |
| start() | |
| } |
| showCorrectionControls | ||
| ? 'max-h-[220px] opacity-100' | ||
| : 'max-h-0 opacity-0 pointer-events-none' |
There was a problem hiding this comment.
Since v-if="showCorrectionControls" was removed, input elements seems to be still focusable by keyboard when collapsed. This adds three invisible focuses and pops up tooltips when you navigates by typing Tab 😅
If we need h-0, I think we could add disabled to avoid focusing on them.
| max="20" | ||
| step="1" | ||
| class="accent-[var(--accent-color,var(--fg-subtle))]" | ||
| /> |
There was a problem hiding this comment.
| /> | |
| :disabled="!showCorrectionControls" | |
| /> |
| max="20" | ||
| step="1" | ||
| class="accent-[var(--accent-color,var(--fg-subtle))]" | ||
| /> |
There was a problem hiding this comment.
| /> | |
| :disabled="!showCorrectionControls" | |
| /> |
| max="30" | ||
| step="1" | ||
| class="accent-[var(--accent-color,var(--fg-subtle))]" | ||
| /> |
There was a problem hiding this comment.
| /> | |
| :disabled="!showCorrectionControls" | |
| /> |
| " | ||
| type="checkbox" | ||
| class="accent-[var(--accent-color,var(--fg-subtle))]" | ||
| /> |
There was a problem hiding this comment.
| /> | |
| :disabled="!showCorrectionControls" | |
| /> |
|
@shuuji3 thank you so much for your thorough review ❤️ |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/Package/TrendsChart.vue (1)
1695-1715: Expose the expand/collapse state to assistive technologies.The toggle currently changes visibility visually, but it does not communicate expanded state or controlled region semantics. Adding
aria-expanded/aria-controlson the button andid/aria-hidden(orinert) on the panel would improve screen-reader clarity.♿ Suggested accessibility refinement
<button type="button" + :aria-expanded="showCorrectionControls" + aria-controls="trends-correction-controls" class="self-start flex items-center gap-1 text-2xs font-mono text-fg-subtle hover:text-fg transition-colors" `@click`="showCorrectionControls = !showCorrectionControls" > @@ - <div + <div + id="trends-correction-controls" + :aria-hidden="!showCorrectionControls" + :inert="!showCorrectionControls" class="overflow-hidden transition-[opacity] duration-200 ease-out" :class=" showCorrectionControls ? 'max-h-[220px] opacity-100' : 'max-h-0 opacity-0 pointer-events-none' " >
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0c93bdb3-7c5e-42be-8ea0-abc1ea0bcd43
📒 Files selected for processing (1)
app/components/Package/TrendsChart.vue
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Package/TrendsChart.vue (1)
1682-1708:⚠️ Potential issue | 🟠 MajorMisplaced ARIA attributes — move to the correct toggle button.
The
aria-expandedandaria-controlsattributes are on the reset date range button, but these attributes should be on the correction controls toggle button (lines 1697-1708). The reset button resets the date range and has no relationship to the correction controls panel.🔧 Proposed fix
Remove ARIA attributes from the reset button:
<button v-if="showResetButton" - :aria-expanded="showCorrectionControls" - aria-controls="trends-correction-controls" type="button" aria-label="Reset date range"Add ARIA attributes to the correction controls toggle button:
<button type="button" + :aria-expanded="showCorrectionControls" + aria-controls="trends-correction-controls" class="self-start flex items-center gap-1 text-2xs font-mono text-fg-subtle hover:text-fg transition-colors" `@click`="showCorrectionControls = !showCorrectionControls" >
🧹 Nitpick comments (1)
app/components/Package/TrendsChart.vue (1)
2175-2178: Consider includingpathelements if main chart line transitions cause CLS.The current selector targets
lineandcircleelements (used in estimation overlays), but if VueUiXy internally transitionspathelements for the main chart lines, those may also need suppression during resize.If you've tested this and confirmed no CLS from path transitions, this is fine as-is. Otherwise, consider:
.no-transition line, -.no-transition circle { +.no-transition circle, +.no-transition path { transition: none !important; }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 30be1dfe-55be-4201-8cb7-616fc1a93a9d
📒 Files selected for processing (1)
app/components/Package/TrendsChart.vue
3 participants
The introduction of the data correction section in the chart modal leads to CLS when toggling the menu.
The fix consists in:
BEFORE:
Enregistrement.de.l.ecran.2026-03-11.a.07.34.48.mov
AFTER:
Enregistrement.de.l.ecran.2026-03-11.a.07.31.29.mov