-
Notifications
You must be signed in to change notification settings - Fork 17
fix: disable restart for followers and remove duplicate tooltip #3036
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
Conversation
|
Skipped: This PR does not contain any of your configured keywords: ( |
| disabled={popoverDisabled} | ||
| > | ||
| {renderButton()} | ||
| <span>{renderButton()}</span> |
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.
why do we need this span?
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.
We need the because the button can be disabled, and a disabled native does not receive pointer/hover/focus events in the browser. The popover wont'be shown on disabled buttons, but we need it (tooltip for followers for example). So if we wrap the button in a non-disabled element (the ), we can attach the popover to the wrapper and keep the button disabled.
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.
Nice work! Let's add a comment so nobody remove this wrapper "to simplify the code".
|
|
||
| function TabletActions(tablet: TTabletStateInfo) { | ||
| const isDisabledRestart = tablet.State === ETabletState.Stopped; | ||
| const isFollower = tablet.Leader === false; |
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.
This logic is used twice in this file. Lets make a helper not to duplicate code.
| isUserAllowedToMakeChanges | ||
| ? i18n('dialog.kill-header') | ||
| : i18n('controls.kill-not-allowed') | ||
| isFollower |
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.
let's now use multi-level ternaries, it's hard to read
#1611
CI Results
Test Status:⚠️ FLAKY
📊 Full Report
Test Changes Summary ⏭️2
⏭️ Skipped Tests (2)
Bundle Size: ✅
Current: 47.08 MB | Main: 47.08 MB
Diff: +0.48 KB (0.00%)
✅ Bundle size unchanged.
ℹ️ CI Information