-
Notifications
You must be signed in to change notification settings - Fork 0
#DKN-205 #DKN-206 #DKN-207 Update the Query Assistant style #102
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: develop
Are you sure you want to change the base?
Conversation
#DKN-206 #DKN-207 Update the filter question to have a single column in the ta…
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.
Pull Request Overview
This PR refactors the Query Assistant component to implement a new design based on ticket #DKN-205. The changes modernize the user interface with improved responsive behavior and better code organization.
- Extracted reusable components and hooks for better maintainability
- Implemented responsive sidebar and preview panel behavior
- Added new design elements including better typography and styling
Reviewed Changes
Copilot reviewed 20 out of 21 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
applications/dknet/frontend/src/utils/types.ts | Adds TypeScript interfaces for component props and configuration |
applications/dknet/frontend/src/utils/helpers.ts | Adds utility function for identifying top match results |
applications/dknet/frontend/src/utils/constants.ts | Defines responsive breakpoints and configuration constants |
applications/dknet/frontend/src/theme/variables.js | Adds new color variables for updated design |
applications/dknet/frontend/src/theme/Theme.tsx | Updates typography, scrollbar, and component styling |
applications/dknet/frontend/src/hooks/*.ts | Creates custom hooks for responsive behavior and filter logic |
applications/dknet/frontend/src/components/FilterAssistantDialog/*.tsx | Refactors dialog components with new responsive layout and design |
applications/dknet/frontend/src/components/ExpandableText.tsx | Adds new expandable text component for question descriptions |
applications/dknet/frontend/.eslintrc.yml | Disables indent ESLint rule |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
const handleResetFilters = () => { | ||
setContext({ | ||
...context, | ||
showAll: false, | ||
filterValues: resetFilters(context.allFilters) | ||
}); | ||
}; |
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 handleResetFilters function should be wrapped in useCallback to prevent unnecessary re-renders of child components that depend on this function.
const handleResetFilters = () => { | |
setContext({ | |
...context, | |
showAll: false, | |
filterValues: resetFilters(context.allFilters) | |
}); | |
}; | |
const handleResetFilters = useCallback(() => { | |
setContext({ | |
...context, | |
showAll: false, | |
filterValues: resetFilters(context.allFilters) | |
}); | |
}, [context, setContext]); |
Copilot uses AI. Check for mistakes.
))} | ||
</List> | ||
{ | ||
!isFiltersEmpty && results.length !== 0 && <Box sx={styles.fadeOverlay(showPreview)} /> |
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 condition !isFiltersEmpty && results.length !== 0
is redundant. If filters are not empty and there are results, results.length !== 0
is implied by the results mapping above.
!isFiltersEmpty && results.length !== 0 && <Box sx={styles.fadeOverlay(showPreview)} /> | |
!isFiltersEmpty && <Box sx={styles.fadeOverlay(showPreview)} /> |
Copilot uses AI. Check for mistakes.
<Box | ||
key={`itemKey_${index}`} | ||
sx={{ | ||
...styles.getItemSx(getCheckedStateForSingle(currentQuestion, data) === 'checked-state'), | ||
...styles.getSingleChoiceItem(shouldExpandHeight, isLastItem), | ||
}} | ||
onClick={(e) => handleOptionClick(e, data)} | ||
> |
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 inline arrow function in onClick will be recreated on every render. Consider using useCallback for handleOptionClick or move it to a separate handler function.
Copilot uses AI. Check for mistakes.
return { | ||
context, | ||
isFiltersEmpty: isFiltersEmpty(), | ||
handleSingleOptionSelect, | ||
isOptionSelectedForMultiple, | ||
isOptionSelectedForSingle, | ||
filterValues: context.filterValues, | ||
results: context.results | ||
}; |
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 isFiltersEmpty()
function is called on every render. Consider memoizing this value with useMemo since it depends on context.filterValues.
Copilot uses AI. Check for mistakes.
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.
Pull Request Overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 3 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
flex: 1, | ||
overflow: 'auto', | ||
height: '100%', | ||
maxWidth: props.maxWidth || '100%', |
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 maxWidth
prop is not defined in the component's props interface. Consider adding proper TypeScript typing for the props parameter to improve type safety.
Copilot uses AI. Check for mistakes.
}, [results]); | ||
|
||
const getPrimaryText = useCallback((el: ResultItem) => { | ||
return !isNaN(el.pctMatch || 0) ? `${el.label} ${el.pctMatch}%` : el.label; |
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 logic for checking !isNaN(el.pctMatch || 0)
is duplicated in multiple places. Consider extracting this into a helper function like hasValidPercentageMatch
for better maintainability.
Copilot uses AI. Check for mistakes.
export const isTopMatch = (item: ResultItem, index: number, results: ResultItem[]): boolean => { | ||
return (index === 0 || item.pctMatch === results[0]?.pctMatch) && !isNaN(item.pctMatch || 0); |
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 same !isNaN(item.pctMatch || 0)
pattern is used here and in PreviewPanel. Consider creating a shared utility function for this validation logic.
export const isTopMatch = (item: ResultItem, index: number, results: ResultItem[]): boolean => { | |
return (index === 0 || item.pctMatch === results[0]?.pctMatch) && !isNaN(item.pctMatch || 0); | |
export function isValidPctMatch(value: number | undefined | null): boolean { | |
return !isNaN(value ?? 0); | |
} | |
export const isTopMatch = (item: ResultItem, index: number, results: ResultItem[]): boolean => { | |
return (index === 0 || item.pctMatch === results[0]?.pctMatch) && isValidPctMatch(item.pctMatch); |
Copilot uses AI. Check for mistakes.
No description provided.