-
Notifications
You must be signed in to change notification settings - Fork 432
Fix React DOM nesting and prop warnings #5335
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
redisinsight/ui/src/components/base/layout/sidebar/SideBarItemIcon.tsx
Outdated
Show resolved
Hide resolved
redisinsight/ui/src/pages/browser/components/redisearch-key-list/RediSearchIndexesList.tsx
Outdated
Show resolved
Hide resolved
6a80c64 to
7a37365
Compare
7a37365 to
f234c03
Compare
Code Coverage - Frontend unit tests
Test suite run success5487 tests passing in 704 suites. Report generated by 🧪jest coverage report action from 2078473 |
redisinsight/ui/src/pages/browser/components/redisearch-key-list/RediSearchIndexesList.tsx
Outdated
Show resolved
Hide resolved
redisinsight/ui/src/pages/browser/components/redisearch-key-list/RediSearchIndexesList.tsx
Outdated
Show resolved
Hide resolved
cf28d76 to
f5ffe11
Compare
...nsight/ui/src/pages/browser/modules/key-details/components/no-key-selected/NoKeySelected.tsx
Show resolved
Hide resolved
| toastContent.message = ( | ||
| <Text size="M" variant="semiBold"> | ||
| <ColorText color={color}>{message}</ColorText> | ||
| <Text size="M" variant="semiBold" component="span"> |
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.
Not a big deal, but I would use div not span
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.
Valid! The toast message, however, has some wrapper already that is: <p> {everything is rendered here} </p>, so we endup with: <p><div>...</div></p> which triggers a warning.
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.
There must be a simpler fix up the chain that is simpler than adding a prop to each Text component here
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.
I removed a few of them, so WDT now?
redisinsight/ui/src/components/notifications/success-messages.tsx
Outdated
Show resolved
Hide resolved
redisinsight/ui/src/pages/browser/components/key-tree/KeyTree.tsx
Outdated
Show resolved
Hide resolved
Add component prop to Text components to prevent invalid HTML nesting. Text renders as <p> by default which cannot contain block elements.
Add key prop to mapped elements to prevent React warning.
Replace props spread with explicit row prop to prevent passing invalid attributes to checkbox element.
a053e9f to
5018da2
Compare
What
Fixes React
validateDOMNestingconsole warnings caused by invalid HTML nesting (e.g.,<div>inside<p>,<p>inside<p>).Changes
component="div"orcomponent="span"prop toTextcomponents that wrap block-level elements, preventing invalid<p>nesting<div>inside<p>warning by addingcomponent="div"to Text wrappercomponent="span"InlineRowstyled component withas: 'span'for inline flex layoutscomponentprops to Text elements in GroupsViewWrapper, ConsumersViewWrapper, StreamDataViewWrapperNot Fixed (Out of Scope)
The
customColorprop warning on SVG elements comes from the external@redis-ui/icons_multicolorpackage. This would be handled in a separate PR.There's one additional warning related to nested buttons inside RediSearchIndexesList. The @redis-ui Accordion component renders a button internally, and when we pass a button as a child, it creates invalid inside nesting. This requires refactoring on the @redis-ui side and will be addressed separately.
Testing
validateDOMNestingwarnings should appearCloses N/A