[KEYCLOAK-11211] Create a Wrapper for the Dropdown component#6275
[KEYCLOAK-11211] Create a Wrapper for the Dropdown component#6275abstractj wants to merge 1 commit intokeycloak:masterfrom abstractj:KEYCLOAK-11211
Conversation
|
@ssilvert this is what I've been talking for Dropdowns. The context and motivation are here: https://issues.jboss.org/browse/KEYCLOAK-11211 |
ssilvert
left a comment
There was a problem hiding this comment.
Overall, I don't see how this component adds a lot of value over stand-alone Dropdown.
The test is to first use the PF Dropdown by itself. Then if you find yourself repeating the same code in another context and you can factor out the common stuff, you make a new component. If that is what you did then I think it's fine.
| import {Msg} from './Msg'; | ||
|
|
||
| interface DropdownWrapperProps { | ||
| dropdownWrapperItems: any[]; |
There was a problem hiding this comment.
This should not be any[]. I think it needs to be Array< DropdownItem | DropdownSeperator>
You can look at the type definition for Dropdown to see.
| public render(): React.ReactNode { | ||
| const { isDropdownOpen } = this.state; | ||
| return ( | ||
| <React.Fragment> |
There was a problem hiding this comment.
The React.Fragment wrapper is optional here because you already have a single tag to return.
| interface DropdownWrapperProps { | ||
| dropdownWrapperItems: any[]; | ||
| dropdownWrapperTitle?: string; | ||
| onDropdownSelect: () => void; |
There was a problem hiding this comment.
This needs to specify the event param. Otherwise, you don't know what was selected.
@ssilvert that's was exactly the motivation to write this. In the application's screen we have 2 dropdowns, which means as far as I know functions like Another alternative is to follow the same approach you did for LocaleSelectors ( What works best for you?
|
I think #2. Just my opinion, but I don't think what you have does enough to be a general purpose component. It limits the available options in Dropdown without giving you much in return. If later you find that you need it in another context then you can decide to pull it out into its own file or just export it. |
What you said to me makes perfect sense, I'm going to stick with #2. Thanks for your time reviewing this. For now I'm going to close this PR and make the changes necessary inside the ApplicationsPage. |
No description provided.