-
Notifications
You must be signed in to change notification settings - Fork 667
shared: PopoverStatus - support active icon and showing popover without header #2453
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
shared: PopoverStatus - support active icon and showing popover without header #2453
Conversation
09e9893 to
51a6370
Compare
|
/retest |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we extract this into PopoverStatusProps to avoid complex in-line types?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If hideHeader is true we'll be passing headerContent={false} to the Popover component.
I think it's more correct to omit headerContent instead of passing headerContent={false} but since the type is React.ReactNode I think this code is OK.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we use React.useCallback for function props, like onHide and onShow ?
51a6370 to
7b9f8bd
Compare
|
fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should extract these memoized callbacks into separate variables.
Also, any function used within these callbacks should be treated as their dependency.
const [isActive, setActive] = React.useState(false);
const onHide = React.useCallback(() => setActive(false), [setActive]);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
7b9f8bd to
94f2aed
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: suomiy, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
needed for #2452