fix(Page): remove unecessary onPageResize default prop #7811
Conversation
|
Preview: https://patternfly-react-pr-7811.surge.sh A11y report: https://patternfly-react-pr-7811-a11y.surge.sh |
Hyperkid123
force-pushed
the
remove-listener-default-prop
branch
from
a0fa31c to
07b1f4c
Compare
| isManagedSidebar: false, | ||
| isBreadcrumbWidthLimited: false, | ||
| defaultManagedSidebarIsOpen: true, | ||
| onPageResize: (): void => null, |
There was a problem hiding this comment.
Even though this technically unnecessary, we consider changes to default values breaking. Could you please update the PR to change the logic at conditional instead.
WE can open a separate issue to remove needed default in a breaking change release.
There was a problem hiding this comment.
@tlabaj we can't change the logic at the condition level. The condition itself is correct but the supplied values are not. The condition checks for the function's existence. And the function is always defined. So the condition is actually buggy and can never be false. That is what breaks the environment.
I've put all the details into the linked issue.
The only other option is adding a new flag that would just disable the observer. But again, looking at the condition logic, the existance of a prop is what should decide the outcome.
There was a problem hiding this comment.
One other alternative that would allow proper opting-out of the page resizing behavior without potentially causing breaking changes: we could update the type for onPageResize to be a union of the current type and null or something to that effect.
@Hyperkid123 do you have any thoughts regarding that potential approach? I know it isn't ideal, but I think it would be the simplest approach to getting you unblocked until our next breaking change release.
There was a problem hiding this comment.
Cool, I am on board with that. Let me update the PR then.
There was a problem hiding this comment.
@Hyperkid123 I opened issue #7861 to address the default value for our next major release.
Hyperkid123
force-pushed
the
remove-listener-default-prop
branch
2 times, most recently
from
e2d5f23 to
e12678b
Compare
| * Returns object { mobileView: boolean, windowSize: number } | ||
| */ | ||
| onPageResize?: (object: any) => void; | ||
| onPageResize?: (object: any) => void | null; |
There was a problem hiding this comment.
I think this typing would still require a function, but the function could just return void | null and that the following type is what you'd really need.
| onPageResize?: (object: any) => void | null; | |
| onPageResize?: ((object: any) => void) | null; |
There was a problem hiding this comment.
Oh yeah, you are correct. This just changes the return type
Hyperkid123
force-pushed
the
remove-listener-default-prop
branch
from
e12678b to
5b9405c
Compare
Hyperkid123
requested review from
wise-king-sullyman
and removed request for
kmcfaul
wise-king-sullyman
left a comment
wise-king-sullyman
left a comment
There was a problem hiding this comment.
This should be good! I will approve and merge as soon as it passes CI 🙂
4 participants
Partially helps with: #7810