-
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
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. |
WalkthroughAdds a test "version 49 props - no logo" to ExpandedHeader.test.tsx covering a primary header without a logo image. Updates PrimaryHeaderSlot to detect whether LogoSlot contains image data and expose that as Sequence Diagram(s)sequenceDiagram
participant Client as Client/Renderer
participant Resolver as resolveData
participant Primary as PrimaryHeaderSlot
participant Logo as LogoSlot
rect rgb(247,250,255)
Note over Client,Resolver: Render request lifecycle
end
Client->>Resolver: request header data
Resolver-->>Client: resolved data (slots + conditionalRender)
Client->>Primary: render header with resolved data
Primary->>Logo: inspect LogoSlot content
Logo-->>Primary: returns image data (present or absent)
Primary->>Primary: set conditionalRender.hasLogoImage
alt hasLogoImage == true
Primary->>Logo: render LogoSlot (auto height)
else hasLogoImage == false
Primary->>Logo: render LogoSlot inside wrapper (min-height:100px)
end
Primary-->>Client: composed header DOM
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Repository UI Review profile: CHILL Plan: Pro ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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