style: updated margins and font styles for editor #5978
Conversation
WalkthroughThe changes in this pull request primarily focus on updating the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
web/core/components/pages/editor/title.tsx (1)
28-30: Well-structured implementation of typography changes.Good practices observed:
- Consistent use of Tailwind utility classes
- Maintains responsive design with dynamic font sizing
- Preserves existing functionality while improving aesthetics
Consider extracting these typography values into a theme configuration for better maintainability and consistency across the application.
packages/editor/src/styles/variables.css (1)
Margin changes are being removed, not added as mentioned in PR objectives
The git diff shows that margin-related changes are actually being removed from the codebase, contrary to the PR objectives that mentioned updating vertical margins. Specifically:
- Multiple
margin-topvalues (2rem, 1.4rem, 1rem, 0.25rem) are being removed- Several
margin-bottomvalues (4px, 1px) are being removed- Only one new margin rule is being added:
margin: 0 !importantThis indicates a discrepancy between the PR objectives and the actual changes being made.
🔗 Analysis chain
Line range hint
1-95: Verify the location of margin updates mentioned in PR objectives.While the font style changes are well-implemented, the PR objectives mention updating vertical margins (y-margins) for editor blocks. However, these margin changes are not present in this file.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for margin-related changes in editor styles echo "Searching for margin-related CSS changes in editor files..." rg -t css "margin|spacing" "packages/editor/src/styles/"Length of output: 2222
Script:
#!/bin/bash # Let's check the git diff to see if there are any margin-related changes in this PR echo "Checking git diff for margin-related changes..." git diff HEAD~1 -- "packages/editor/src/styles/" | grep -E '(\+|\-).*margin' # Also check for any new margin-related CSS variables that might have been added echo -e "\nChecking for margin-related CSS variables..." rg -t css "var\(.*margin.*\)" "packages/editor/src/styles/"Length of output: 718
packages/editor/src/core/extensions/extensions.tsx (1)
81-81: Consider keeping margin-based spacing for horizontal rulesThe change from
my-4topy-4switches from margin to padding-based spacing. While both provide visual spacing, margins are typically preferred for horizontal rules as they:
- Better handle margin collapse with adjacent elements
- Follow conventional HTML5 semantic spacing patterns
- Provide more predictable spacing behavior
Consider reverting to margin-based spacing or verify that padding specifically addresses a layout requirement:
- class: "py-4 border-custom-border-400", + class: "my-4 border-custom-border-400",packages/editor/src/styles/editor.css (1)
312-320: Consider documenting the margin reset strategyThe use of
!importantto reset margins is quite aggressive. While this appears to be part of a transition to padding-based spacing, it would be beneficial to:
- Document why this approach was chosen
- Add a comment explaining that spacing is handled via padding
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
packages/editor/src/core/extensions/core-without-props.ts(1 hunks)packages/editor/src/core/extensions/extensions.tsx(1 hunks)packages/editor/src/core/extensions/read-only-extensions.tsx(1 hunks)packages/editor/src/styles/editor.css(2 hunks)packages/editor/src/styles/variables.css(4 hunks)web/core/components/pages/editor/title.tsx(1 hunks)
🔇 Additional comments (7)
web/core/components/pages/editor/title.tsx (1)
28-30: Font weight and line height adjustments look good, but consider accessibility.
The changes align well with the PR objectives to improve readability:
- Increased line heights provide better vertical spacing
- Bolder font weight enhances visual hierarchy
However, ensure that the contrast between the title and background remains sufficient with the new font weight, especially for users with visual impairments.
Let's verify the color contrast ratio for the title:
packages/editor/src/styles/variables.css (1)
49-49: Well-chosen typography for large font code blocks!
The code font size (0.85rem) maintains readability while being appropriately smaller than regular text (1rem). The line height (1.5rem) creates comfortable spacing and maintains vertical rhythm with regular text.
Also applies to: 59-59
packages/editor/src/core/extensions/core-without-props.ts (1)
49-49: 🛠️ Refactor suggestion
Consider using margin instead of padding for horizontal rule spacing.
Changing from my-4 (margin) to py-4 (padding) for a horizontal rule might not achieve the desired spacing effect. Horizontal rules (<hr>) are typically styled with margins rather than padding since they are self-closing elements, and padding might not render consistently across browsers.
Consider reverting to margin-based spacing:
- class: "py-4 border-custom-border-400",
+ class: "my-4 border-custom-border-400",Let's verify the visual impact of this change:
packages/editor/src/core/extensions/read-only-extensions.tsx (1)
70-70: Consider the implications of switching from margin to padding.
The change from my-4 (margin-y) to py-4 (padding-y) might affect the visual spacing differently:
- Margins collapse between adjacent elements while paddings don't
- This could lead to inconsistent spacing in different contexts (e.g., when horizontal rules are placed next to other block elements)
Let's verify the spacing consistency:
Consider using CSS custom properties for spacing to maintain consistency:
- class: "py-4 border-custom-border-400",
+ class: "my-block-spacing border-custom-border-400",This would allow for easier maintenance and consistency of spacing across the editor.
✅ Verification successful
Based on the analysis of the codebase, I can now provide a final response:
The change from margin to padding is consistent with the codebase's styling approach
The switch from my-4 to py-4 for horizontal rules is actually beneficial because:
- The change is consistently applied across all editor configurations (
core-without-props.ts,read-only-extensions.tsx, andextensions.tsx) - The Tailwind configuration shows custom spacing scale where both margin and padding use the same scale (0.9rem for spacing-4)
- The editor's typography styles already handle spacing between block elements using padding in most cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any other margin/padding classes used with horizontal rules
# and verify consistency across the codebase
# Search for other horizontal rule configurations
rg -l "CustomHorizontalRule" | xargs rg "my-|py-" -A 5 -B 5
# Look for related CSS classes that might affect spacing
rg "\.prose.*hr|horizontal-rule" --type css
Length of output: 2766
Script:
#!/bin/bash
# Let's check for any existing margin/padding styles in the CSS files
# and also look for any Tailwind configuration files that might define
# custom spacing for horizontal rules
# Search for CSS files with hr styling
rg "hr\s*{[^}]*}" --type css -A 5 -B 5
# Search for Tailwind configuration files
fd "tailwind.config" --exec cat {}
# Search for any prose styling configuration
rg "prose" --type css -A 5 -B 5
Length of output: 26030
packages/editor/src/core/extensions/extensions.tsx (1)
Line range hint 1-182: Verify implementation of all style updates mentioned in PR objectives
The PR objectives mention updates to font sizes, weights, and line-heights for text blocks, as well as reduced font size for code blocks. However, these changes are not visible in the current file. Please ensure that:
- Font style updates are implemented somewhere in the codebase
- Code block font size reduction is properly applied
Let's check for any related style changes in the codebase:
packages/editor/src/styles/editor.css (2)
325-383: LGTM! Well-structured heading hierarchy
The heading styles demonstrate:
- Consistent vertical rhythm (28px/20px top padding based on heading level)
- Uniform bottom padding (4px)
- Good use of CSS variables for maintainable typography
15-15: Verify text readability after line-height removal
While setting a base font-weight is good, the removal of line-height from .ProseMirror could affect text readability. Ensure that all text elements have appropriate line-height values set individually.
✅ Verification successful
Line-height is properly maintained throughout text elements
The codebase shows comprehensive line-height definitions:
- Base text has 1.5 line-height
- All heading levels (h1-h6) use custom CSS variables for line-height
- Regular text and lists have dedicated line-height variables
- The only zero line-height appears to be intentional, likely for a specific UI element
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if line-height is properly set for all text elements
rg "line-height:" packages/editor/src/styles/
Length of output: 774
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/editor/src/styles/editor.css (2)
15-15: Consider removing redundantfont-weightdeclarationThe
font-weight: 400;may be unnecessary if this is the default value or already inherited. Removing redundant declarations can help keep the CSS clean and maintainable.
312-320: Avoid using!importantto reset marginsUsing
!importantcan make the CSS harder to override and maintain. Consider increasing selector specificity or adjusting the cascade to achieve the desired effect without relying on!important.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/editor/src/styles/editor.css(2 hunks)packages/editor/src/styles/variables.css(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/editor/src/styles/variables.css
🔇 Additional comments (2)
packages/editor/src/styles/editor.css (2)
325-325: Verify consistency of padding-top values for headings
The padding-top is set to 28px for headings h1 to h4, while h5 and h6 use 20px. Please confirm that this difference aligns with the design specifications.
Also applies to: 336-336, 347-347, 358-358, 369-369, 380-380
398-400: Duplicate paragraph spacing rules still exist
As previously noted, there are duplicate rules affecting paragraph spacing:
- Within
.prose :where(p): Lines 398-400 setpadding-bottom: 4px; - Standalone rule
p + p: Lines 410-412 setpadding-top: 8px !important;
Consider consolidating these rules to avoid conflicts and simplify maintenance.
Also applies to: 410-412
Improvements:
Summary by CodeRabbit
New Features
CustomHorizontalRulein the editor, improving vertical spacing.Bug Fixes
Chores