-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
fix: 15752 invalid tooltip on hovering the select component #15788
base: main
Are you sure you want to change the base?
fix: 15752 invalid tooltip on hovering the select component #15788
Conversation
DCO Assistant Lite bot All contributors have signed the DCO. |
I have read the DCO document and I hereby sign the DCO. |
recheck |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
@alisonjoseph and @guidari, PR is ready for review. Thanks 😊 |
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.
@mHuzefa Hii :) thank you for contributing, is there a way you can fix the merge conflicts? Thank you! |
0ea8370
to
f1c0f67
Compare
@guidari Have changed the values to match what you suggested. @andreancardona I have resolved the merged conflicts although I am sorry that I forgot to rebase my main with upstream and all commits came in this branch so I had to forced push to rewrite the changes. That's why it added other reviewers too. |
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.
Thanks for taking a look at this - I have a couple suggestions
) as HTMLSelectElement | null; | ||
if (defaultValue) { | ||
const defaultOption: HTMLOptionElement | null | undefined = | ||
selectElement?.querySelector(`option[value=${defaultValue}]`); |
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.
Is there a way to declaratively get the value from children
instead of using this imperative selector?
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.
Hi @tay1orjones, removed the imperative selector as I realized it was not needed since we could get value through selectedIndex
@@ -255,6 +272,10 @@ const Select = React.forwardRef(function Select( | |||
size: 'mini', | |||
}); | |||
} | |||
useEffect(() => { | |||
selectDefaultTitle(defaultValue); |
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 doesn't need to be in a function, just place the code directly in the useEffect. It'll be easier to maintain this way being all in once place and not having to scroll up to find the function definition.
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.
Done
469888d
to
397ac89
Compare
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, []); |
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 needs to run on every render of the component? If so, I don't think it needs to be in a useEffect
. If the select
has a ref
on it, can we use the ref to get the selected index/element instead of using document.getElementById?
We have a mergeRefs
helper function that can be used to combine the forwarded ref
and a local ref inside the component for this use if it's needed.
Closes #15752
Fixed the invalid title/no title in Select component on hovering
New
selectDefaultTitle
which gets select element by it's id. ifdefaultValue
is passed, it gets the option withdefaultValue
from select, otherwise get option by using theselectedIndex
Changed
selectedIndex
inselect.options
selectDefaultTitle
inuseEffect
Select
withdefaultValue
'Option 2', expected result should be the title 'Option 2'Testing / Reviewing