Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(@toss/react): Add root Props ImpressionArea #283

Merged
merged 4 commits into from
Jul 10, 2023

Conversation

ssi02014
Copy link
Contributor

@ssi02014 ssi02014 commented Jul 4, 2023

Overview

I worked on adding "root" props to the "Impression Area".
"root" props is one of the default options for "Intersection Observer". How about adding a corresponding "root" prop?

I think it's better to cover a wider range of cases by providing a default option that can be utilized.

For example, The root option has issues with iframes.

Of course, by default, the Viewport is applied as root (null), which I agree is appropriate in most cases. So, it would be great if you could review it.

Also, I don't think the code I've been working on will cause problems with older versions of the code. 🤗

PR Checklist

  • I read and included theses actions below
  1. I have read the Contributing Guide
  2. I have written documents and tests, if needed.

@netlify
Copy link

netlify bot commented Jul 4, 2023

👷 Deploy request for slash-libraries pending review.

Visit the deploys page to approve it

Name Link
🔨 Latest commit 644b121

Copy link
Member

@okinawaa okinawaa left a comment

Choose a reason for hiding this comment

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

This is a good improvement.

What kind of situation did you actually need to give a root to?(in real programming)

Could you write a test code to verify this change?

@ssi02014
Copy link
Contributor Author

ssi02014 commented Jul 5, 2023

@ChanhyukPark-Tech
In fact, root is an option that most of the time is not needed in real programming..!

However, I recently had an experience with storybook where rootMargin didn't work because I didn't specify root.
when I specified root as document, "rootMargin" worked fine.

(Not exactly a storybook issue, but more of an iframe issue🤮)

Please refer to the following videos that I shared! 🙏

root: null (Unusual behavior 🥲)

ezgif.com-gif-maker.7.mov

root: document (Normal behavior 😄)

ezgif.com-gif-maker.8.mov

I found a documentation related to the issue.

Looking at this issue again, I see that I gave a completely bogus answer to the initial question. It's NOT true that root margin is only applied for the same-document case (and the spec doesn't say that). Root margin will be applied in the same-origin cross-document case (i.e., target element inside a same-origin iframe, and observer using the implicit root).

However, the root margin is only applied to the top-level container, which in the case of the implicit root is the scrolling viewport of the top-level document. The iframe element is also a clipping container, and the root margin is NOT applied when computing clipping from the iframe element's boundary.

If you want the root margin to apply to the iframe element's boundary, then you'll have to use an observer with an explicit root, which should be document.scrollingElement within the iframe. If you also need to compute intersection with the top-level viewport (the implicit root), that will require a second observer.

Solution

const observer = new IntersectionObserver(callback, {
  root: document, // (*)
  rootMargin: '300px 0px 300px 0px',
});

Additionally, in my opinion, it seems quite difficult to write test code with the root option applied using the existing test code settings… 😱

As a result, There's a clear problem with storybook(actually iframe), adding the root option doesn't have any side effects, and it's the default option for Intersection Observer, so I don't think it's a bad idea to add it.

Copy link
Member

@okinawaa okinawaa left a comment

Choose a reason for hiding this comment

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

Thank you for preparing a good example and reference and producing this pr.

Someone might need this option

@okinawaa okinawaa merged commit 980a7e1 into toss:main Jul 10, 2023
@ssi02014
Copy link
Contributor Author

@ChanhyukPark-Tech
I'm glad this pull request got approved and merged.
Also, thanks for your review. 🙂

@okinawaa
Copy link
Member

@ssi02014
I learn a lot from you because you always create good PR. Thank you!😄

@ssi02014 ssi02014 deleted the fix/ImpressionArea branch July 23, 2023 15:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants