-
-
Notifications
You must be signed in to change notification settings - Fork 730
Code blocks have an optional text-wrap toggle #2009
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
Conversation
|
WalkthroughThis update introduces two new SVG icon components, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CodeBlock
participant HighlightCode
User->>CodeBlock: Render with showTextWrapping prop
CodeBlock->>User: Display code and toolbar
User->>CodeBlock: Click wrap toggle button
CodeBlock->>CodeBlock: Toggle isWrapped state
CodeBlock->>HighlightCode: Render code with isWrapped prop
HighlightCode->>User: Display code with/without wrapping
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/webapp/app/components/code/CodeBlock.tsx (2)
340-340
: Shared wrap state between main and modal viewsThe same
isWrapped
state is used for both the main view and modal view, but the toggle is only available in the main view. This seems intentional based on the PR objectives, but users might expect to be able to toggle wrapping in the modal view as well.Consider either:
- Making the wrap state independent for the modal view
- Adding the toggle button to the modal view
- Adding a note in the PR description about this design decision
Also applies to: 395-395
486-486
: Note about indentation preservationAs mentioned in the PR objectives, this implementation doesn't preserve code indentation when text wrapping is enabled. This is a known limitation rather than a bug.
If preserving indentation is important for code readability, consider exploring CSS solutions like
white-space: pre-line
combined with custom indentation preservation logic, though this would be a more complex enhancement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webapp/app/assets/icons/TextInlineIcon.tsx
(1 hunks)apps/webapp/app/assets/icons/TextWrapIcon.tsx
(1 hunks)apps/webapp/app/components/code/CodeBlock.tsx
(13 hunks)apps/webapp/app/components/runs/v3/PacketDisplay.tsx
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
apps/webapp/app/components/code/CodeBlock.tsx (3)
apps/webapp/app/components/primitives/Tooltip.tsx (4)
TooltipProvider
(118-118)Tooltip
(118-118)TooltipTrigger
(118-118)TooltipContent
(118-118)apps/webapp/app/assets/icons/TextInlineIcon.tsx (1)
TextInlineIcon
(1-41)apps/webapp/app/assets/icons/TextWrapIcon.tsx (1)
TextWrapIcon
(1-34)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - pnpm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: e2e / 🧪 CLI v3 tests (ubuntu-latest - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (11)
apps/webapp/app/assets/icons/TextInlineIcon.tsx (1)
1-41
: Icon component looks well-implementedThe TextInlineIcon component is well-structured with proper SVG viewBox, stroke settings, and accessibility considerations. The component properly accepts an optional className prop for styling flexibility.
apps/webapp/app/assets/icons/TextWrapIcon.tsx (1)
1-34
: Icon component follows best practicesThe TextWrapIcon component is well-implemented with proper SVG attributes and styling. It maintains consistency with TextInlineIcon in terms of API and styling approach.
apps/webapp/app/components/runs/v3/PacketDisplay.tsx (1)
47-47
: Good integration of the text wrapping featureThe addition of the
showTextWrapping
prop to the CodeBlock component in the default case is a clean way to enable the text wrapping functionality for JSON content.apps/webapp/app/components/code/CodeBlock.tsx (8)
6-6
: Icon imports properly organizedThe imports for the new icon components are correctly positioned in the file.
Also applies to: 12-12
36-37
: Good JSDoc for the new propThe JSDoc comment for the
showTextWrapping
prop clearly describes its purpose.
191-191
: State and prop setup looks goodThe
showTextWrapping
prop with default value offalse
and theisWrapped
state are properly initialized.Also applies to: 211-211
273-291
: Well-implemented toggle button with tooltipThe text wrapping toggle button is well-implemented with:
- Conditional rendering based on
showTextWrapping
prop- Clean state toggling implementation
- Appropriate tooltip that changes based on the current state
- Proper use of icons to indicate the current/next state
340-340
: Consistent prop passing to HighlightCodeThe
isWrapped
state is properly passed to both instances of the HighlightCode component (main view and modal view).Also applies to: 395-395, 451-451, 463-463
345-349
: Dynamic styling based on wrap stateThe conditional styling for the container and pre elements based on the
isWrapped
state is well-implemented. The use ofcn()
utility makes the code clean and readable.Also applies to: 354-360
476-488
: Refactored styling logic improves maintainabilityThe refactoring of style classes into variables (
containerClasses
andpreClasses
) improves code maintainability and reduces duplication. Good use of thecn()
utility for conditional class application.
530-532
: Consider potential issues with line number positioningThe sticky positioning of line numbers in wrapped mode is a good approach, but be aware that it might have inconsistent behavior across different browsers or in complex layouts.
Consider testing this functionality in different browsers (especially older ones) to ensure the sticky positioning of line numbers works as expected in wrapped mode.
Also applies to: 540-543
User requested improvement: CodeBlock component has a button that toggles between "Text wrap" and "Text inline".
Notes:
whitespace-pre-wrap
andbreak-word
using isWrappedSummary by CodeRabbit
New Features
Enhancements