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

fix: move target node for event logger a few nodes down #190

Merged
merged 2 commits into from
Jun 17, 2020

Conversation

smeijer
Copy link
Member

@smeijer smeijer commented Jun 17, 2020

What:

I've changed the node to which DomEvents listens for events. The node was positioned inside DomEvents, now we're listening to the Preview root instead.

Why:
Listening to a node that wraps the entire preview pane, causes unrelated events to be logged. This is because the Preview component doesn't only hold the raw markup, it also contains Scrollable and AutoSizer (resize detectors).

The AutoSizer triggers on page load, so it caused the first events to be directly added on-page load. This change fixes that as well.

How:
Use the callback ref to bind the events directly on the correct root node, instead of wrapping the entire <Preview> component.

Checklist:

  • Tests
  • Ready to be merged

@smeijer smeijer requested a review from marcosvega91 June 17, 2020 13:56
@smeijer smeijer force-pushed the feature/fix-height branch from 17eedaa to bc2cb8b Compare June 17, 2020 14:04
@smeijer
Copy link
Member Author

smeijer commented Jun 17, 2020

@marcosvega91 & @sumeesh879, can you check if this fixes the problem that was mentioned in #178 (comment)?

@sumeeshn
Copy link

@smeijer I still see the issue in this PR preview.

Screen Shot 2020-06-17 at 10 31 10 AM

I think you'll still need to compare innerRef height with outerRef height.

@marcosvega91's last commit 97840de on #178 seems to have fixed it.

@smeijer
Copy link
Member Author

smeijer commented Jun 17, 2020

Yeah, that commit looked like it. But it isn't really. As your screenshot also shows, for some reason items 0, 1 and 2 aren't rendered. There is a bit more going on than just those ref heights.

*edit
Hmm... That does fix it. But I have no clue why.

@smeijer smeijer changed the title fix: initial height of event log pane fix: move target node for event logger a few nodes down Jun 17, 2020
@smeijer smeijer force-pushed the feature/fix-height branch from 73dbb67 to 0ba33e4 Compare June 17, 2020 17:33
@smeijer smeijer removed the request for review from marcosvega91 June 17, 2020 17:41
@smeijer smeijer merged commit 63c2940 into develop Jun 17, 2020
@smeijer smeijer deleted the feature/fix-height branch June 17, 2020 17:42
@smeijer smeijer self-assigned this Jun 17, 2020
@smeijer smeijer added the bug Something isn't working label Jun 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants