Skip to content
This repository has been archived by the owner on Jan 11, 2023. It is now read-only.

[Preview] Don't allow popover to move outside editor boundaries #6654

Merged
merged 2 commits into from
Jul 20, 2018

Conversation

darkwing
Copy link
Contributor

Fixes Issue: #5134

Supercedes #6537

42651217-63e67cfe-85d4-11e8-9e9f-29eca1d5fef9

@darkwing darkwing changed the title Popover 2 [Preview] Don't allow popover to move outside editor boundaries Jul 17, 2018
@darkwing
Copy link
Contributor Author

Superceded by #6654

@darkwing darkwing closed this Jul 17, 2018
@darkwing
Copy link
Contributor Author

Do'h, closed wrong PR

this.state = {
top: 0
};
}
Copy link
Contributor

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

Copy link
Contributor

@jasonLaster jasonLaster left a 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;
Copy link
Contributor

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because we don't want this:

screen shot 2018-07-17 at 4 33 54 pm

Copy link
Contributor Author

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 :)

@darkwing
Copy link
Contributor Author

The saga continues with the test failure. I stubbed in a noop function for onPopoverCoords but that leads to the last test, tooltop won't display above the target when insufficient space, failing, which didn't happen in my original PR. I can look again tomorrow with a fresh brain/sleep.

<div
className="gap"
key="gap"
/>
<h1>
Toolie!
</h1>
Copy link
Contributor

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

@darkwing darkwing merged commit fecd1b3 into firefox-devtools:master Jul 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants