-
Notifications
You must be signed in to change notification settings - Fork 24
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
PSP-9630: Historical File on Header - ability to hide Historical File Numbers when headers is expanded #4593
Conversation
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4593 |
const displayedItems = items.slice(0, displayedItemsLength); | ||
|
||
console.log(items.length - displayedItemsLength); |
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.
please remove console log here
const [displayedItemsLength, setDisplayedItemsLength] = useState( | ||
!isExpanded ? maxCollapsedLength : displayedPerChunk ?? items.length, | ||
); |
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.
in the old logic "maxCollapsedLength ?? items.length"
was protecting against maxCollapsedLength being undefined/null. In the new logic I don't see the same. Shouldn't we keep that logic in the new one?
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.
Yes, will make those changes
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4593 |
@@ -10,6 +10,7 @@ export interface IExpandableTextListProps<T> { | |||
keyFunction: (item: T, index: number) => string; | |||
delimiter?: ReactElement | string; | |||
maxCollapsedLength?: number; | |||
displayedPerChunk?: number; |
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.
The naming seems a bit confusing. If I understood the logic this prop would limit the amount of rows to show when the component is in the expanded state - right?
Perhaps a name like "maxExpandedLength" would better match the intention? It would also match the other maxCollapsedLength in the naming
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4593 |
1 similar comment
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4593 |
See CodeCov Report Here: https://app.codecov.io/github/bcgov/psp/pull/4593 |
Additionally to implement the UI/UX solution suggested by Ana, I also added a max-height on the header to avoid the Property Details Header to expand and occupy all the screen height.