Skip to content

🖍 Bento Lightbox: Customizable styles#31565

Merged
caroqliu merged 10 commits intoampproject:masterfrom
caroqliu:lightbox-css
Dec 15, 2020
Merged

🖍 Bento Lightbox: Customizable styles#31565
caroqliu merged 10 commits intoampproject:masterfrom
caroqliu:lightbox-css

Conversation

@caroqliu
Copy link
Copy Markdown
Contributor

@caroqliu caroqliu commented Dec 11, 2020

Partial for #31052

@caroqliu caroqliu requested a review from dvoytenko December 11, 2020 18:33
@caroqliu caroqliu marked this pull request as ready for review December 11, 2020 20:55
position: fixed;
visibility: hidden;
box-sizing: border-box;
background-color: rgba(0, 0, 0, 0.9);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Weird that we have a background color with transparency. Are we ok with some very bright content seen through?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think so, given publishers can change this now. It should be somewhat translucent so end users know there is more content on the page than the lightbox modal they have open - but maybe not necessary if we do not allow it to be open on load. I was actually thinking 0.9 might be too opaque.

className={classes.lightboxContainWrapper}
part="lightbox"
wrapperClassName={`${classes.defaultStyles} ${classes.wrapper}`}
wrapperStyle={{visibility: 'hidden'}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FYI: this is not great. But I guess we will fix in a followup PRs. Ideally we always set "final" values on the rendering level and modify whatever is needed for animation during the animation side effect.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Moved to jss wrapper class - verified this is overridden by inline styles applied during animations.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still seeing it here. But what I actually want is a little different:

  1. No wrapperStyle={{visibility: hidden}}.
  2. No visibility:hidden (or opposite, only visibility:visible) in JSS.
  3. All animation nuances are applied within the corresponding useLayoutEffect. E.g. the opening animation starts by setting style to visibility:hidden and finishes by setting it to visible.

Basically, if we disable animation effect - the styles are fully correct from the start.

Copy link
Copy Markdown
Contributor Author

@caroqliu caroqliu Dec 14, 2020

Choose a reason for hiding this comment

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

Done - however I removed the visibility for the wrapper styles in JSS since it is visible by default. In another PR it could be nice to move some of the style setting callbacks to the doc or to useCallback so they are not defined every time they are used.

Copy link
Copy Markdown
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

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

LGTM with one ask.

className={classes.lightboxContainWrapper}
part="lightbox"
wrapperClassName={`${classes.defaultStyles} ${classes.wrapper}`}
wrapperStyle={{visibility: 'hidden'}}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Still seeing it here. But what I actually want is a little different:

  1. No wrapperStyle={{visibility: hidden}}.
  2. No visibility:hidden (or opposite, only visibility:visible) in JSS.
  3. All animation nuances are applied within the corresponding useLayoutEffect. E.g. the opening animation starts by setting style to visibility:hidden and finishes by setting it to visible.

Basically, if we disable animation effect - the styles are fully correct from the start.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants