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 😊 |
|
@mHuzefa Hii :) thank you for contributing, is there a way you can fix the merge conflicts? Thank you! |
andreancardona
added
status: merge conflict 🚫
and removed
status: ready for review 👀
labels
mHuzefa
requested review from
tay1orjones,
a team and
laurenmrice
as code owners
mHuzefa
force-pushed
the
invalid-tooltip-on-hover
branch
from
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. |
| ) as HTMLSelectElement | null; | ||
| if (defaultValue) { | ||
| const defaultOption: HTMLOptionElement | null | undefined = | ||
| selectElement?.querySelector(`option[value=${defaultValue}]`); |
There was a problem hiding this comment.
Is there a way to declaratively get the value from children instead of using this imperative selector?
There was a problem hiding this comment.
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.
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.
mHuzefa
force-pushed
the
invalid-tooltip-on-hover
branch
from
469888d
to
397ac89
Compare
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); |
There was a problem hiding this comment.
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
selectDefaultTitlewhich gets select element by it's id. ifdefaultValueis passed, it gets the option withdefaultValuefrom select, otherwise get option by using theselectedIndexChanged
selectedIndexinselect.optionsselectDefaultTitleinuseEffectSelectwithdefaultValue'Option 2', expected result should be the title 'Option 2'Testing / Reviewing