-
Notifications
You must be signed in to change notification settings - Fork 1
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
Refactor Suggestions components for clarity #165
base: main
Are you sure you want to change the base?
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.
This is looking great! Thanks for working on this, makes the code way more readable. I left small comments here and there
|
||
// eslint-disable-next-line func-names | ||
const DefaultRenderSectionItemsList: RenderSectionItemsList = function ({ section }) { | ||
const { getSectionProps } = useContext(CioAutocompleteContext); |
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.
Since you already created a new hook, should we use it here? useCioAutocompleteContext
const DefaultRenderSectionItemsList: RenderSectionItemsList = function ({ section }) { | ||
const { getSectionProps } = useContext(CioAutocompleteContext); | ||
const { displayName } = section; | ||
const sectionTitle = camelToStartCase(displayName || getSectionTitle(section)); |
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 think we don't want to run displayName
through camelToStartCase.
If the user set it to be camelcase, we should leave it as is
highlightSearchTerm, | ||
}: SectionItemTextProps) { | ||
export default function SectionItemText({ item, highlightSearchTerm }: SectionItemTextProps) { | ||
const { query } = useCioAutocompleteContext(); |
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.
Q: Any reason why we don't move displaySearchTermHighlights
here and stop passing it around?
This introduces no logic changes. It's just organizing code into Components for clarity.
Before
SectionItem
After
Added
useCioAutocompleteContext
to make it clear what context is being used