-
Notifications
You must be signed in to change notification settings - Fork 406
RI-6841: Add close button for unsupported key types #4401
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
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.
PR Overview
This PR adds a close button for unsupported key types and refactors duplicate styling and logic across multiple key detail components by introducing a new wrapper component. Key changes include:
- Creation of TextDetailsWrapper to consolidate common functionality.
- Updating UnsupportedTypeDetails, TooLongKeyNameDetails, DynamicTypeDetails, and ModulesTypeDetails to use the new wrapper and standardize how the close button is handled.
- Making minor improvements such as changing the disabledRefreshButtonMessage prop to optional and updating onCloseKey signature in KeyDetailsHeader.
Reviewed Changes
File | Description |
---|---|
TextDetailsWrapper.tsx | New component to wrap detail components and provide a close button. |
UnsupportedTypeDetails.tsx | Refactored to use TextDetailsWrapper and accept an onClose prop. |
TooLongKeyNameDetails.tsx | Updated to use TextDetailsWrapper and pass onClose prop. |
DynamicTypeDetails.tsx | Updated to pass onClose props to children components. |
AutoRefresh.tsx | Changed disabledRefreshButtonMessage to be an optional prop. |
ModulesTypeDetails.tsx | Refactored to use TextDetailsWrapper for consistency. |
KeyDetailsHeader.tsx | Updated onCloseKey signature to remove the unused key argument. |
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
redisinsight/ui/src/pages/browser/modules/key-details/components/text-details-wrapper/TextDetailsWrapper.tsx:10
- [nitpick] Consider using a more generic data-testid value for TextDetailsWrapper instead of one that references only 'too-long-key-name', since this wrapper is used across multiple key detail types.
<div className={styles.container} data-testid="too-long-key-name-details">
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.
PR Overview
This PR adds a close button to key details panels by introducing a new wrapper component, TextDetailsWrapper, and refactoring existing key detail components to use it.
- Introduced TextDetailsWrapper to consolidate close button functionality across detail components.
- Removed repetitive styling and logic from UnsupportedTypeDetails, TooLongKeyNameDetails, and ModulesTypeDetails, and updated tests accordingly.
- Adjusted prop types for onClose functions in components and tests, and made disabledRefreshButtonMessage optional in the AutoRefresh component.
Reviewed Changes
File | Description |
---|---|
TextDetailsWrapper.spec.tsx | Updated tests for rendering and onClose behavior of the new wrapper |
TextDetailsWrapper.tsx | New wrapper component that encapsulates the close button and layout for detail components |
TooLongKeyNameDetails.spec.tsx | Modified test to pass required onClose prop |
UnsupportedTypeDetails.spec.tsx | Modified test to pass required onClose prop and refactored component usage |
ModulesTypeDetails.spec.tsx | Updated component to receive onClose prop and refactored UI structure |
AutoRefresh.tsx | Made disabledRefreshButtonMessage prop optional |
DynamicTypeDetails.tsx | Propagated onClose prop to all detail components for consistency |
KeyDetailsHeader.tsx | Updated onCloseKey prop signature to remove unused argument |
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (1)
redisinsight/ui/src/pages/browser/modules/key-details/components/text-details-wrapper/TextDetailsWrapper.spec.tsx:16
- [nitpick] Duplicate test description found. Consider renaming one of the tests (e.g., distinguishing the onClose trigger test) to clarify the unique purpose of each test case.
it('should render children correctly', () => {
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.
PR Overview
This PR adds a close button for key details panels when no header is available, improving the user experience for unsupported key types, module details, and keys with long names. Key changes include the introduction of a shared TextDetailsWrapper component for close button functionality, updates to related detail components and their tests to propagate an onClose prop, and a minor update to the auto-refresh component to make its disabled-refresh message optional.
Reviewed Changes
File | Description |
---|---|
redisinsight/ui/src/pages/browser/modules/key-details/components/text-details-wrapper/TextDetailsWrapper.tsx | Introduces a new wrapper component that renders a close button and wraps children. |
redisinsight/ui/src/pages/browser/modules/key-details/components/text-details-wrapper/TextDetailsWrapper.spec.tsx | Adds tests for rendering children and invoking the onClose callback. |
redisinsight/ui/src/pages/browser/modules/key-details/components/too-long-key-name-details/TooLongKeyNameDetails.tsx | Refactors the component to use TextDetailsWrapper and accept an onClose prop. |
redisinsight/ui/src/pages/browser/modules/key-details/components/too-long-key-name-details/TooLongKeyNameDetails.spec.tsx | Updates the test to pass a mock onClose function. |
redisinsight/ui/src/pages/browser/modules/key-details/components/unsupported-type-details/UnsupportedTypeDetails.tsx | Updates the component to use TextDetailsWrapper with an onClose prop. |
redisinsight/ui/src/pages/browser/modules/key-details/components/unsupported-type-details/UnsupportedTypeDetails.spec.tsx | Updates the test to supply a mock onClose callback. |
redisinsight/ui/src/pages/browser/modules/key-details/components/modules-type-details/ModulesTypeDetails.tsx | Refactors to use TextDetailsWrapper and include the onClose prop. |
redisinsight/ui/src/pages/browser/modules/key-details/components/modules-type-details/ModulesTypeDetails.spec.tsx | Adjusts the test to include the onClose callback. |
redisinsight/ui/src/pages/browser/modules/key-details/components/dynamic-type-details/DynamicTypeDetails.tsx | Propagates the onClose callback to child components based on key type. |
redisinsight/ui/src/components/auto-refresh/AutoRefresh.tsx | Makes the disabledRefreshButtonMessage prop optional. |
redisinsight/ui/src/pages/browser/modules/key-details-header/KeyDetailsHeader.tsx | Updates the onCloseKey interface and its usage to remove the unused key parameter. |
Copilot reviewed 14 out of 14 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
redisinsight/ui/src/pages/browser/modules/key-details-header/KeyDetailsHeader.tsx:144
- The onCloseKey function signature has been updated to no longer accept a key parameter. Please ensure that all caller components have been updated accordingly to avoid potential runtime issues.
onClick={() => onCloseKey()}
redisinsight/ui/src/components/auto-refresh/AutoRefresh.tsx:42
- [nitpick] Consider providing a default fallback for disabledRefreshButtonMessage to ensure that the UI displays a user-friendly message when none is provided.
disabledRefreshButtonMessage?: string
redisinsight/ui/src/pages/browser/modules/key-details/components/dynamic-type-details/DynamicTypeDetails.tsx:39
- Ensure that props.onCloseKey is always defined when passed to child components like TooLongKeyNameDetails to prevent runtime errors when the onClose callback is invoked.
return <TooLongKeyNameDetails onClose={ props.onCloseKey }/>
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.
overall LGTM
...src/pages/browser/modules/key-details/components/modules-type-details/ModulesTypeDetails.tsx
Outdated
Show resolved
Hide resolved
|
||
export interface KeyDetailsHeaderProps { | ||
onCloseKey: (key: RedisResponseBuffer) => void | ||
onCloseKey: () => void |
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.
good catch, if this really is not used
… too long key name details
…er needed on key close.
Co-authored-by: Krum Tyukenov <krum.tyukenov@gmail.com>
92ca541
to
a18a79c
Compare
For some keys, their details page does not contain the keys header so the close button is not there.
in this PR such close button is added so the details panel could be closed.
Types affected:
Changes applied:
Side fixes: