Bento: add container to the lightbox 1.0 #33488
Conversation
| 'onBeforeOpen': this.toggle_.bind(this, true), | ||
| 'onAfterClose': this.toggle_.bind(this, false), | ||
| 'onBeforeOpen': () => this.beforeOpen_(), | ||
| 'onAfterOpen': () => this.afterOpen_(), |
There was a problem hiding this comment.
Do we only need onAfterOpen because the element is not rendered when !mounted? Can we do <ContainWrapper style={mounted && {display: 'none'}}> instead of mounted && <ContainWrapper>?
There was a problem hiding this comment.
We could do display: none, but the goal was specifically to not render contents when the lightbox is not open to make it more React-style.
There was a problem hiding this comment.
To add to this. I generally like the idea that the React component looks more inline with that style. And the cost of an event handler seems not too much of a price to pay?
There was a problem hiding this comment.
I'm also confused why we can't setAsContainer in onBeforeOpen. Shouldn't it handle when the scroller eventually becomes visible?
Maybe we shouldn't be calling onBeforeOpen until it's actually mounted?
There was a problem hiding this comment.
It's due to this call:
const scroller = this.element.shadowRoot.querySelector('[part=scroller]');
this.setAsContainer(scroller);
The lightbox itself is not rendered until it's open and thus <div part="scroller"> is not loaded either. So doing that querySelector will return null. For the container it's important to use the actual element with overflow:auto|scroll for the intersection observer to provide the accurate values.
There was a problem hiding this comment.
I will merge before the long weekend. But we will have another opportunity to discuss this with the sidebar component.
There was a problem hiding this comment.
Sidebar recently replaced the mounted && <ContainWrapper> with the stylized mounting approach in #33244.
There was a problem hiding this comment.
Yep. Found it as well. And I think that's ok - these components are not exactly the same. Though I'd also like to understand the new sidebar a bit better.
Follow up on #33281 for 1.0 version.
Partial for #31915.
Partial for #31540.