-
Notifications
You must be signed in to change notification settings - Fork 1
fix: contentDetail 화면에서 header position이 absolute로 설정되어 있어, 콘텐츠 블록하고 겹치던 문제 해결 #113
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 set of changes updates styling and metadata across several frontend components and configuration files. Modifications include adjustments to container heights, padding, and positioning in React components, updates to the meta description and image preload attributes in the HTML, and removal of inline comments and emojis from a GitHub Actions workflow file. No logic or control flow was altered. Changes
Sequence Diagram(s)No sequence diagram generated as all changes are limited to styling, metadata, and comment/formatting updates. Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 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 (
|
E2E Test & Lighthouse Report
|
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 (1)
src/features/setting/SettingSection.jsx (1)
16-18: Margin reduction looks fine but verify small-screen spacingShrinking the vertical margin from
4remto1remalleviates the collision with anabsoluteheader, but it also tightens breathing room on mobile. Double-check the visual density at the smallest breakpoints; you may want to keep a slightly larger top margin (e.g.,2rem) below768px.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/E2ETest_Lighthouse.yml(1 hunks)index.html(1 hunks)src/features/feedback/FeedbackSection.jsx(1 hunks)src/features/setting/SettingSection.jsx(1 hunks)src/layout/Header.jsx(1 hunks)src/layout/MailHeader.jsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: e2e-and-lighthouse
🔇 Additional comments (7)
index.html (2)
7-7: Improved meta description specificity.The updated meta description provides clearer information about Haruhan being a subscription service that delivers daily cultural insights, which is more specific and informative than the previous general description.
14-14: Simplified image preload directive.Removing the
fetchpriority="high"attribute simplifies the preload link. This attribute has limited browser support and may not be necessary for this use case..github/workflows/E2ETest_Lighthouse.yml (1)
81-86: Cleaner PR comment formatting.Removing emojis from the test results and Lighthouse score labels creates a more professional and consistent reporting format while maintaining all functional information.
src/layout/Header.jsx (1)
29-29: Increased vertical padding for better spacing.The padding adjustment from
15px 40pxto20px 40pxincreases vertical spacing in the scrolled header state, which improves visual balance and spacing consistency.src/layout/MailHeader.jsx (1)
10-10: Fixed header overlap issue by changing positioning.Changing from
position: absolutetoposition: relativedirectly addresses the PR objective by preventing the header from overlapping content blocks. With relative positioning, the header remains in the document flow instead of being positioned independently, eliminating the overlap issue mentioned in the contentDetail screen.src/features/feedback/FeedbackSection.jsx (1)
15-15: Adjusted container height for better layout integration.Changing from
min-height: 100vhtoheight: 90vhfixes the container to 90% of viewport height instead of allowing it to expand beyond the full viewport. This adjustment likely complements the header positioning changes by ensuring proper space allocation and preventing layout conflicts.src/features/setting/SettingSection.jsx (1)
8-13:height: 100%may break vertical-centering when the parent has no explicit heightFlex-centering (
align-items: center) relies on the flex container having a determinable cross-axis size.
Withheight: 100%, that size collapses to “auto” unless every ancestor up tohtml, bodyalso hasheight: 100%. If any parent omits it (common in real pages), the section will no longer fill the viewport and the content will sit at the top, re-introducing the overlap problem in a different form.Consider a safer fallback:
- height: 100%; + /* Stretch at least full viewport height while still allowing the content to grow */ + min-height: 100vh;or ensure
html, body, #rootall haveheight: 100%in the global stylesheet.Please confirm which approach is enforced globally so we don’t regress layout on pages that mount this component outside the
contentDetailcontext.
Summary by CodeRabbit
Style
Documentation
Chores