Embeds: show continuous chart side-by-side with CP value#4388
Embeds: show continuous chart side-by-side with CP value#4388
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR enhances embed mode responsiveness across multiple question and chart components by introducing width-aware logic. It adds detection for "wide" embeds (≥500px), conditionally adjusts responsive styling and chart heights based on embed width thresholds, and refines conditional rendering for continuous question headers in embed contexts. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
front_end/src/app/(embed)/questions/components/embed_question_header.tsx (1)
129-138:⚠️ Potential issue | 🟡 Minor
hideLabelprop is alwaysfalsehere — dead codeThe outer guard
!isContinuousQuestion(post.question)already ensures this branch is never reached for continuous questions, soisContinuousQuestion(post.question)is alwaysfalseandhideLabelis alwaysfalse. The prop is either left over from before the guard was added or is a copy-paste of earlier logic. SincehideLabeldefaults tofalse, it can simply be removed.🧹 Proposed cleanup
{isQuestionPost(post) && !isContinuousQuestion(post.question) && ( <div className="[`@container`(max-width:375px)]:hidden"> <QuestionHeaderCPStatus question={post.question as QuestionWithForecasts} size="md" - hideLabel={isContinuousQuestion(post.question)} colorOverride={predictionColor} chartTheme={theme?.chart} /> </div> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/app/`(embed)/questions/components/embed_question_header.tsx around lines 129 - 138, The hideLabel prop passed to QuestionHeaderCPStatus is dead code because the surrounding conditional already excludes continuous questions; remove the hideLabel={isContinuousQuestion(post.question)} attribute from the JSX branch that renders QuestionHeaderCPStatus (the component rendered when isQuestionPost(post) && !isContinuousQuestion(post.question)) so the component relies on its default hideLabel behavior instead.
🧹 Nitpick comments (2)
front_end/src/app/(main)/questions/[id]/components/question_view/forecaster_question_view/question_header/question_header_cp_status.tsx (2)
158-168: Consider extracting the nested height ternary to a named constantThe three-level nested ternary is functional but dense. A named variable makes the intent immediately readable.
♻️ Optional refactor
+const chartHeight = + hideLabel && size === "lg" + ? 120 + : isEmbedBelow376 + ? 32 + : isEmbedWide + ? 90 + : 50; + <MinifiedContinuousAreaChart question={question} data={continuousAreaChartData} - height={ - hideLabel && size === "lg" - ? 120 - : isEmbed - ? isEmbedBelow376 - ? 32 - : isEmbedWide - ? 90 - : 50 - : 50 - } + height={chartHeight}Note: the refactored version also drops the redundant outer
isEmbedcheck — when not in embed mode,isEmbedBelow376andisEmbedWideare bothfalse, so the nested ternary already falls through to50, which is the same as the current: 50fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/app/`(main)/questions/[id]/components/question_view/forecaster_question_view/question_header/question_header_cp_status.tsx around lines 158 - 168, Extract the nested three-level ternary used for the height prop into a named constant (e.g., const computedHeight) and use that constant in place of the inline ternary; compute computedHeight from the existing identifiers hideLabel, size, isEmbed, isEmbedBelow376, and isEmbedWide and simplify by removing the redundant outer isEmbed branch since isEmbedBelow376/isEmbedWide will be false when not embedded, preserving the same fallbacks (120, 32, 90, 50) and improving readability in the QuestionHeaderCPStatus component where the height prop is set.
109-116:max-w-[130px]andmax-w-[195px]conflict whensize === "md" && isEmbedWideWhen
size === "md"andisEmbedWideare both true, bothmax-w-[130px](line 109 – triggered bysize === "md") andmax-w-[195px](line 116 – triggered byisEmbedWide) are emitted into the class string simultaneously. The correct value (max-w-[195px]) only wins becausecnusestailwind-mergeto resolve the conflict. Making the first condition explicitly excludeisEmbedWideremoves the implicit dependency:🛡️ Proposed fix to make the intent explicit
- "max-w-[130px]": - size === "md" || (isEmbed && !isEmbedBelow376 && !isEmbedWide), + "max-w-[130px]": + (size === "md" && !isEmbedWide) || (isEmbed && !isEmbedBelow376 && !isEmbedWide),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@front_end/src/app/`(main)/questions/[id]/components/question_view/forecaster_question_view/question_header/question_header_cp_status.tsx around lines 109 - 116, The "max-w-[130px]" class condition currently triggers for size === "md" even when isEmbedWide is true, causing a conflicting max-w with "max-w-[195px]"; update the predicate for the "max-w-[130px]" entry in question_header_cp_status.tsx so it explicitly excludes isEmbedWide (e.g., make the condition require size === "md" && !isEmbedWide (and keep the existing isEmbed branch), referencing the variables size, isEmbed, isEmbedBelow376, and isEmbedWide used in that class list) so only the intended max-w is emitted and tailwind-merge no longer has to resolve the conflict.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@front_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsx`:
- Around line 324-344: The parent wrapper for the OverlayableTimeline's
responsive container queries is missing the Tailwind container marker so the
@[23.5rem]: variants never activate; add the `@container` class to the closest
parent div that currently has className="@[23.5rem]:items-center
@[23.5rem]:gap-3 group relative flex" (the wrapper around overlay and timeline)
so the @[23.5rem]: rules take effect, and apply the same `@container` addition to
the sibling parent wrappers where similar @[23.5rem]: queries appear (the other
divs at the same block using cn(...) for overlay and timeline); ensure the
`@container` is present on the nearest ancestor of OverlayableTimeline so the
wide-mode layout can render.
---
Outside diff comments:
In `@front_end/src/app/`(embed)/questions/components/embed_question_header.tsx:
- Around line 129-138: The hideLabel prop passed to QuestionHeaderCPStatus is
dead code because the surrounding conditional already excludes continuous
questions; remove the hideLabel={isContinuousQuestion(post.question)} attribute
from the JSX branch that renders QuestionHeaderCPStatus (the component rendered
when isQuestionPost(post) && !isContinuousQuestion(post.question)) so the
component relies on its default hideLabel behavior instead.
---
Nitpick comments:
In
`@front_end/src/app/`(main)/questions/[id]/components/question_view/forecaster_question_view/question_header/question_header_cp_status.tsx:
- Around line 158-168: Extract the nested three-level ternary used for the
height prop into a named constant (e.g., const computedHeight) and use that
constant in place of the inline ternary; compute computedHeight from the
existing identifiers hideLabel, size, isEmbed, isEmbedBelow376, and isEmbedWide
and simplify by removing the redundant outer isEmbed branch since
isEmbedBelow376/isEmbedWide will be false when not embedded, preserving the same
fallbacks (120, 32, 90, 50) and improving readability in the
QuestionHeaderCPStatus component where the height prop is set.
- Around line 109-116: The "max-w-[130px]" class condition currently triggers
for size === "md" even when isEmbedWide is true, causing a conflicting max-w
with "max-w-[195px]"; update the predicate for the "max-w-[130px]" entry in
question_header_cp_status.tsx so it explicitly excludes isEmbedWide (e.g., make
the condition require size === "md" && !isEmbedWide (and keep the existing
isEmbed branch), referencing the variables size, isEmbed, isEmbedBelow376, and
isEmbedWide used in that class list) so only the intended max-w is emitted and
tailwind-merge no longer has to resolve the conflict.
...t_end/src/components/detailed_question_card/detailed_question_card/continuous_chart_card.tsx
Outdated
Show resolved
Hide resolved
ncarazon
force-pushed
the
feat/embeds-continuous-chart-update
branch
from
8a91670 to
8f7bc36
Compare
ncarazon
force-pushed
the
feat/embeds-continuous-chart-update
branch
from
8f7bc36 to
a083d1f
Compare
🚀 Preview EnvironmentYour preview environment is ready!
Details
ℹ️ Preview Environment InfoIsolation:
Limitations:
Cleanup:
|
1 participant
This PR updates the embed view for continuous questions to display the minified chart side-by-side with the community prediction value, instead of using an overlay layout.
Before:
After:
Summary by CodeRabbit
Bug Fixes
Improvements