fix(LoginPage): allow brand props passthrough make brand optional #12194
Conversation
WalkthroughLoginPage now accepts an optional Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/react-core/src/components/LoginPage/LoginPage.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/components/LoginPage/LoginPage.tsx (1)
packages/react-core/src/components/Brand/Brand.tsx (2)
BrandProps(7-35)Brand(37-82)
⏰ 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). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (3)
packages/react-core/src/components/LoginPage/LoginPage.tsx (3)
5-5: LGTM!The BrandProps import is correctly added and necessary for the new
brandImgPropsprop type definition.
24-25: LGTM!The new optional
brandImgPropsprop correctly enables consumers to pass additional Brand properties (e.g., width, height) through the LoginPage component, addressing issue #11970.
55-55: LGTM!The prop destructuring correctly includes the new
brandImgPropsparameter.
|
Preview: https://pf-react-pr-12194.surge.sh A11y report: https://pf-react-pr-12194-a11y.surge.sh |
… without props Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
LGTM! Similar to a comment I left on your Tabs PR, but would you feel comfortable adding some unit tests for these changes?
Yes, certainly |
Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @packages/react-core/src/components/LoginPage/__tests__/LoginPage.test.tsx:
- Around line 40-45: The test for LoginPage's brandImgProps only checks src/alt
but must verify passthrough of additional attributes; update the test that calls
LoginPage with brandImgProps including extra fields (e.g., width, height,
className, data-*), render the component, then assert those attributes are
applied to the Brand image element (by querying the Brand/img and checking
getAttribute or classList) or include them in the snapshot to confirm forwarding
from the brandImgProps to the Brand component.
🧹 Nitpick comments (3)
packages/react-core/src/components/LoginPage/__tests__/LoginPage.test.tsx (3)
30-33: Consider adding explicit assertions alongside snapshots.While snapshot testing is valid, adding an explicit query for the Brand component (or its underlying img element) would make the test intent clearer and provide more informative failure messages.
📋 Suggested enhancement
test('brand is absent without brandImgSrc and brandImgProps.src', () => { - const { asFragment } = render(<LoginPage loginTitle="Log into your account" />); + const { asFragment, container } = render(<LoginPage loginTitle="Log into your account" />); + expect(container.querySelector('img[class*="pf-v6-c-brand"]')).toBeNull(); expect(asFragment()).toMatchSnapshot(); });
35-38: Consider adding explicit assertions alongside snapshots.Similar to the previous test, adding an explicit assertion that the Brand/img element exists would improve test clarity.
📋 Suggested enhancement
test('brand is present with brandImgSrc prop', () => { - const { asFragment } = render(<LoginPage brandImgSrc="Brand src" loginTitle="Log into your account" />); + const { asFragment, container } = render(<LoginPage brandImgSrc="Brand src" loginTitle="Log into your account" />); + const brandImg = container.querySelector('img[class*="pf-v6-c-brand"]'); + expect(brandImg).toBeInTheDocument(); + expect(brandImg).toHaveAttribute('src', 'Brand src'); expect(asFragment()).toMatchSnapshot(); });
30-45: Consider testing the edge case when both brandImgSrc and brandImgProps.src are provided.The implementation spreads
brandImgPropsafter settingsrcandalt, meaningbrandImgPropsvalues should override. Adding a test for this scenario would document the expected precedence behavior and prevent regressions.📋 Suggested edge case test
test('brandImgProps.src overrides brandImgSrc when both provided', () => { const { container } = render( <LoginPage brandImgSrc="fallback.png" brandImgProps={{ src: 'primary.png', alt: 'Primary logo' }} loginTitle="Log into your account" /> ); const brandImg = container.querySelector('img[class*="pf-v6-c-brand"]'); expect(brandImg).toHaveAttribute('src', 'primary.png'); expect(brandImg).toHaveAttribute('alt', 'Primary logo'); });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/react-core/src/components/LoginPage/__tests__/__snapshots__/LoginPage.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
packages/react-core/src/components/LoginPage/__tests__/LoginPage.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
packages/react-core/src/components/LoginPage/__tests__/LoginPage.test.tsx (2)
packages/react-integration/demo-app-ts/src/components/demos/LoginPageDemo/LoginPageDemo.tsx (1)
render(59-164)packages/react-core/src/components/LoginPage/LoginPage.tsx (1)
LoginPage(50-99)
⏰ 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). (2)
- GitHub Check: Build, test & deploy
- GitHub Check: Build
packages/react-core/src/components/LoginPage/__tests__/LoginPage.test.tsx
Show resolved
Hide resolved
…ame attributes Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-core/src/components/LoginPage/__tests__/LoginPage.test.tsx (2)
30-38: Consider asserting Brand presence/absence directly instead of snapshots. Snapshots here can become noisy when unrelated markup changes.Possible refactor
test('brand is absent without brandImgSrc and brandImgProps.src', () => { - const { asFragment } = render(<LoginPage loginTitle="Log into your account" />); - expect(asFragment()).toMatchSnapshot(); + render(<LoginPage loginTitle="Log into your account" />); + expect(screen.queryByRole('img')).not.toBeInTheDocument(); }); test('brand is present with brandImgSrc prop', () => { - const { asFragment } = render(<LoginPage brandImgSrc="Brand src" loginTitle="Log into your account" />); - expect(asFragment()).toMatchSnapshot(); + render(<LoginPage brandImgSrc="Brand src" loginTitle="Log into your account" />); + expect(screen.getByRole('img')).toHaveAttribute('src', 'Brand src'); });
54-65: Good precedence test; consider adding a guard for “brandImgProps without src” to prevent empty-src regressions. E.g., pass{ className: 'x' }only and assert Brand is not rendered.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
packages/react-core/src/components/LoginPage/__tests__/__snapshots__/LoginPage.test.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
packages/react-core/src/components/LoginPage/__tests__/LoginPage.test.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). (2)
- GitHub Check: Build
- GitHub Check: Build, test & deploy
🔇 Additional comments (2)
packages/react-core/src/components/LoginPage/__tests__/LoginPage.test.tsx (2)
2-2: Good switch toscreenfor DOM assertions (more idiomatic RTL).
40-52: Nice coverage ofbrandImgPropspassthrough (aria-label + className) with concrete assertions.
…d customization Signed-off-by: Mohamed Fall <ps.hackmaster@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/react-core/src/components/LoginPage/examples/LoginPageBasic.tsx (1)
114-127: Example demonstrates new feature correctly.The use of
brandImgPropswithsrc,alt, andclassNameeffectively showcases the new capability for consumers to customize the Brand component.One optional consideration: Since
brandImgSrcandbrandImgAltremain valid props (withbrandImgPropstaking precedence), you might want to keep the "Basic" example using the simplerbrandImgSrc/brandImgAltpattern and demonstratebrandImgPropsin a separate example or alongside them to show precedence behavior. This would help consumers understand both usage patterns.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-core/src/components/LoginPage/examples/LoginPage.mdpackages/react-core/src/components/LoginPage/examples/LoginPageBasic.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). (2)
- GitHub Check: Build, test & deploy
- GitHub Check: Build
🔇 Additional comments (1)
packages/react-core/src/components/LoginPage/examples/LoginPage.md (1)
35-36: LGTM!Clear documentation of the new
brandImgPropsfeature and its precedence overbrandImgSrc. This aligns well with the PR objectives.
3 participants
Changes:
Summary by CodeRabbit
New Features
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.