-
Notifications
You must be signed in to change notification settings - Fork 758
[Preview] Don't allow popover to move outside editor boundaries #6654
Conversation
Superceded by #6654 |
Do'h, closed wrong PR |
this.state = { | ||
top: 0 | ||
}; | ||
} |
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 wonder if this can be:
export class Popup extends Component<Props, State> {
// stuff
state: State = { top: 0 }
// stuff
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.
Looks good!
@@ -5,12 +5,9 @@ | |||
.popover .preview-popup { | |||
background: var(--theme-body-background); | |||
width: 350px; | |||
min-height: 80px; |
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 don't we need a min?
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.
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.
Also note that this 80px min wasn't even used; it was overridden lower in the same selector :)
The saga continues with the test failure. I stubbed in a noop function for |
<div | ||
className="gap" | ||
key="gap" | ||
/> | ||
<h1> | ||
Toolie! | ||
</h1> |
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.
not sure if this moving is a big deal
Fixes Issue: #5134
Supercedes #6537