feat: extend PopperProps in derivative component types
#11934
Conversation
|
Preview: https://patternfly-react-pr-11934.surge.sh A11y report: https://patternfly-react-pr-11934-a11y.surge.sh |
logonoff
force-pushed
the
menucontainer
branch
3 times, most recently
from
fb8e369 to
cf608bc
Compare
logonoff
changed the title
PopperProps for MenuPopperPropsPopperProps in derivative component types
| @@ -4,7 +4,7 @@ section: components | |||
| subsection: menus | |||
| cssPrefix: pf-v6-c-select | |||
| propComponents: | |||
| ['Select', 'SelectOption', 'SelectGroup', 'SelectList', 'MenuToggle', 'SelectToggleProps', 'SelectPopperProps'] | |||
There was a problem hiding this comment.
Overall, I think this isn't introducing a breaking change. but it's possible that these extra interfaces were pulled out the way they were so that they could be documented in our API docs on the website. People won't know without looking at the code how to manipulate these popper features like direction and width if we remove the interfaces all together.
It is possible we could include Popper props on the API docs instead. I'm interested what the team thinks
There was a problem hiding this comment.
I don't think we would want to include PopperProps since it includes some attributes that aren't in the extended types.
What's the motivation for pulling the extended types out?
There was a problem hiding this comment.
SelectPopperProps doesn't show any of the properties inherited from PopperProps, so there is no point in showing it in the documentation
There was a problem hiding this comment.
Now that you added PopperOptions I think you can add that since it won't include the popper attributes that aren't actually on popperProps. It will then list it as the type for popperProps in the component prop documentation with a link
and that link will point to the interface docs of PopperOptions on each derivative component page.
There was a problem hiding this comment.
sounds good. in this case, should we still keep SelectPopperProps, etc. exported as a type, to avoid breaking builds?
There was a problem hiding this comment.
Yeah I think we would need to still keep it around until our next breaking change
| /** Flag to prevent the popper from overflowing its container and becoming partially obscured. */ | ||
| preventOverflow?: boolean; | ||
| } | ||
| export type DropdownPopperProps = Omit<PopperProps, 'trigger' | 'triggerRef' | 'popper' | 'popperRef' | 'isVisible'>; |
There was a problem hiding this comment.
One thing I'm thinking of when I look at these, rather than having Omit<PopperProps, 'trigger' | 'triggerRef' | 'popper' | 'popperRef' | 'isVisible'>; in every derivative component what if we moved that under Popper to DRY things up and then just reexport that in each derivative component under the appropriate name?
logonoff
force-pushed
the
menucontainer
branch
2 times, most recently
from
8f64e28 to
6ac728c
Compare
What: Closes #11929 #11574
Additional issues: