fix(select): Create a generic Id type for Option interface #214
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Add title as an interface prop and set type as ReactNode - Create an enum for Language options and use it in Select and TypeaheadSelect components
|
Anlerkan
reviewed
Nov 28, 2022
src/select/util/selectTypes.ts
Outdated
Comment on lines
15
to
16
| // eslint-disable-next-line @typescript-eslint/no-empty-interface | ||
| interface TypeaheadSelectOption extends Option {} |
There was a problem hiding this comment.
We can use type instead of an interface to avoid disabling this rule.
Suggested change
| // eslint-disable-next-line @typescript-eslint/no-empty-interface | |
| interface TypeaheadSelectOption extends Option {} | |
| type TypeaheadSelectOption = Option; |
There was a problem hiding this comment.
I tried the way you said, but it didn't let me set a generic type to TypeaheadSelectOption 🤔
Anlerkan
reviewed
Nov 28, 2022
src/select/util/selectTypes.ts
Outdated
| id: string; | ||
| interface Option<Id = string> { | ||
| id: Id; | ||
| title: React.ReactNode; |
There was a problem hiding this comment.
Why did we add title property here?
There was a problem hiding this comment.
I think it can be useful for some cases, but I can set it as optional or remove it completely.
Anlerkan
reviewed
Dec 5, 2022
- Use contentRenderer as Select.Item content - Use contentRenderer for tag content as we removed the title prop - Instead of handling the keyword change inside TypeaheadSelect by filtering titles, handle it outside of the component
ErenYLCN
previously approved these changes
Jan 23, 2023
gulcinuras
added
W.I.P
…d to set initial and changeable value
- Set canSelectMultiple to false if options.length is not bigger than 1
- Add testid to use in typeahead-select.test
- Add onClick optional prop to TypeheadSelectTrigger to set the correct classname when the menu is open
- setMenuVisibility to false if shouldCloseOnSelect is true, so that the classnames are correct
- Remove input styles when TypeaheadSelectInput is used inside of the trigger
- Add testid to TypeheadSelectTrigger to test it easily
- Fix most of the test cases for typeahead-select
- Fix classnames
- I used screen.findByText function to find the option but I can update it
Note:- In my opinion, having two classnames for typeahead-select and select is a bit unnecessary. We can just add typeahead-select as main classname and override select classnames if necessary.
- I couldn't fix a11y test case for the typeahead-select.
gulcinuras
requested review from
edizcelik,
yasincaliskan,
akgunberk,
enesdumanli and
jamcry
edizcelik
approved these changes
Feb 23, 2023
tatata96
approved these changes
Sep 12, 2023
Anlerkan
reviewed
Sep 12, 2023
| @@ -47,6 +47,7 @@ function SelectComponent<T extends Option = Option>( | |||
| return ( | |||
| <div | |||
| ref={selectRef} | |||
| data-testid={props.testid} | |||
There was a problem hiding this comment.
In the ideal scenario, I think we should be able to get all "div" props in addition to the SelectProps and pass all of them to the div element. But it is fine for now. Maybe we can discuss it later on and use this logic on other components as well
There was a problem hiding this comment.
I think we should have made a refactor for that component. It looks too complex :(
| <Select.Trigger customClassName={"typeahead-select-trigger"}> | ||
| <Select.Trigger | ||
| customClassName={"typeahead-select-trigger"} | ||
| testid={"TypeaheadSelectTrigger"} |
There was a problem hiding this comment.
Why do we need this testid?
…d as we usually handle the state of the keyword outside of the component
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
titlefromTypeaheadSelectOptionand usecontentRendererprop as children toSelect.IteminTypeaheadSelectSelectandTypeaheadSelectcomponentscontentRendererfortagcontent as we removed thetitlepropinitialKeywordand usecontrolledKeywordto set initial and changeable valuecanSelectMultipleto false ifoptions.lengthis not bigger than 1testidto use intypeahead-select.testonClickoptional prop toTypeheadSelectTriggerto set the correctclassnamewhen the menu is opensetMenuVisibilityto false ifshouldCloseOnSelectis true, so that theclassnamesare correctTypeaheadSelectInputis used inside of the triggertestidtoTypeheadSelectTriggerto test it easilytypeahead-select- Fix classnames
- I used
screen.findByTextfunction to find the option but I can update itNote:
TypeaheadSelectand Select is a bit unnecessary. We can just add typeahead-select as main classname and override select classnames if necessary.a11ytest case for thetypeahead-select.Idas generic type