fix(Popup): fix issue with open prop #3020
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3020 +/- ##
==========================================
+ Coverage 99.92% 99.96% +0.03%
==========================================
Files 169 163 -6
Lines 2802 2745 -57
==========================================
- Hits 2800 2744 -56
+ Misses 2 1 -1
Continue to review full report at Codecov.
|
layershifter
changed the title
| // then set popup coordinates when triggerref is available | ||
| if (_.has(this.props, 'open')) { | ||
| this.setPopupStyle() | ||
| } |
There was a problem hiding this comment.
Should this not only be set if open is truthy? Also, let's link the issue here in the code:
handleTriggerRef = (triggerRef) => {
const { open } = this.props
// ...
// https://github.com/Semantic-Org/Semantic-UI-React/issues/2991
if (open) this.setPopupStyle()
}There was a problem hiding this comment.
Yes, and we should also add open in propTypes list.
There was a problem hiding this comment.
Indeed, we have some work to do in regard to inherited props #1169.
There was a problem hiding this comment.
I'm not sure I understand inherited props. In popup.d.ts file we are already inheriting from PortalProps.
| // element.style.should.have.property('top', '') | ||
| // element.style.should.have.property('left', '') | ||
| // element.style.should.have.property('bottom', '') | ||
| // element.style.should.have.property('right', '') |
| expect(top).to.not.equal('') | ||
| expect(left).to.not.equal('') | ||
| expect(bottom).to.not.equal('') | ||
| expect(right).to.not.equal('') |
There was a problem hiding this comment.
Would be ideal if these made more specific assertions about where it should be positioned opposed to where it should not be positioned.
There was a problem hiding this comment.
Added new assertion expect(element.parentNode).to.equal(document.body)
|
It seems there are some deeper issues here with InnerPortal and ReactDOM.createPortal. See #3029. |
|
I should also note, the tests here are passing but the feature is not working. |
|
@levithomason I've fixed code review comments. Here is the working example 1. Default open prop<Popup trigger={<Button >Opened</Button>} open >test</Popup>2. Change status dynamicallyclass Example extends React.Component {
state ={
open: false,
}
componentDidMount() {
setTimeout(() => {
this.setState({
open: true,
})
}, 5000)
}
render() {
return (
<Container id='introduction-page' style={{ paddingTop: 100 }}>
<Popup trigger={<Button >Opened</Button>} open={this.state.open} >test</Popup>
</Container>
)
}
}I'm facing one issue though, I'm trying to create an example in docs with |
This is the same issue I was having. I'll have to come back to this one and do some digging. |
|
@levithomason Did you get a chance to look into this? |
|
I did not, apologies for the delays. It is on my higher priority list, along with the Dropdown issues we've had recently. Working on these a bit today as well. |
|
What's the status on this? @levithomason |
|
For those who need this functionality, this is a regression and if you rollback to this version it'll work: |
|
This needs tested in a browser to ensure it is working as expected. Last I pulled this fork and updated one of the examples to have Are there any others who are getting the correct results from this PR? |
|
@msrikanth508 any chance you can change the base branch here from your |
|
I've reached some progress in #3029 |
|
@levithomason I've created new branch from my commits. |
|
Fixed in #3029. |
Issue: After rendering
triggerprop there is no way to calculate popup position styles, hence it is rendering somewhere in the DOMFix: Calling
setPopupStylefunc in trigger handler to calculate popup positions based ontriggerprop elementFixes #2991.