-
Notifications
You must be signed in to change notification settings - Fork 0
fix: added min-height for logo slot #968
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
base: main
Are you sure you want to change the base?
Conversation
|
Warning: Component files have been updated but no migrations have been added. See https://github.com/yext/visual-editor/blob/main/packages/visual-editor/src/components/migrations/README.md for more information. |
WalkthroughThe PR adds a test "version 50 props - no logo" for ExpandedHeader and updates PrimaryHeaderSlot to detect and propagate a new Sequence Diagram(s)sequenceDiagram
participant Client as Client/Renderer
participant Resolver as resolveData
participant Primary as PrimaryHeaderSlot
participant Logo as LogoSlot
rect rgb(242,248,255)
Note over Client,Resolver: Page render starts
end
Client->>Resolver: request resolved header data
Resolver-->>Client: resolved data (includes slots)
Client->>Primary: render with resolved data
Primary->>Resolver: derive hasLogoImage from LogoSlot data
Resolver-->>Primary: conditionalRender.hasLogoImage
alt hasLogoImage == true
Primary->>Logo: render LogoSlot (auto height)
Logo-->>Primary: image rendered
else hasLogoImage == false
Primary->>Logo: render LogoSlot inside wrapper (min-height:100px)
Logo-->>Primary: placeholder/empty rendered
end
Primary-->>Client: composed header DOM
Possibly related PRs
Suggested reviewers
Pre-merge checks✅ Passed checks (2 passed)
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (2)
⏰ 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). (4)
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 |
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: 2
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (29)
packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 10 props - secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 10 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 11 props - secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 15 props - fixed header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 15 props - sticky header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 20 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 41 props - no secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 41 props - with secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 48 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[desktop] version 50 props - no logo.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] default props (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 10 props (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 10 props - secondary header (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 11 props - secondary header (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 15 props - fixed header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 15 props - sticky header (after interactions).pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 20 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[mobile] version 50 props - no logo.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] default props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 10 props - secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 11 props - secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 15 props - fixed header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 15 props - sticky header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 20 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 41 props - no secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 41 props - with secondary header.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 48 props.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**packages/visual-editor/src/components/testing/screenshots/ExpandedHeader/[tablet] version 50 props - no logo.pngis excluded by!**/*.png,!packages/visual-editor/src/components/testing/screenshots/**
📒 Files selected for processing (2)
packages/visual-editor/src/components/header/ExpandedHeader.test.tsxpackages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx
⏰ 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). (4)
- GitHub Check: call_unit_test / unit_tests (24.x)
- GitHub Check: call_unit_test / unit_tests (22.x)
- GitHub Check: call_unit_test / unit_tests (20.x)
- GitHub Check: semgrep/ci
🔇 Additional comments (2)
packages/visual-editor/src/components/header/PrimaryHeaderSlot.tsx (1)
110-118: Verify the logic after refactoring the image detection.The wrapper structure and conditional
minHeightlogic align with the PR objectives. However, the effectiveness depends on correctly detecting image presence. Once the image detection approach is refactored (see comments on lines 91-92 and 97-105), verify that:
- The 100px
minHeightis applied only whenLogoSlotis empty or contains no image.- No layout shift occurs during the initial render.
packages/visual-editor/src/components/header/ExpandedHeader.test.tsx (1)
943-1088: LGTM! Test case properly covers the no-logo scenario.The new test case follows the established pattern of existing version tests and correctly validates the header behavior when
LogoSlotis empty (line 960). This should exercise the conditionalminHeightlogic added inPrimaryHeaderSlot.tsxand ensure consistent spacing when no logo is present.
Ticket - OPAQF-57
Screen.Recording.2026-01-02.at.5.21.16.PM.mov