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: reset code snippet when try to append a new code #352

Merged
merged 2 commits into from
Apr 23, 2021

Conversation

ValGeorgiev
Copy link
Contributor

@ValGeorgiev ValGeorgiev commented Apr 21, 2021

The solution: Clean the code section before appending new clicked query. We first check if there are some child nodes and if there are, we remove them and append the new code. It's strange that there are methods for getting firstChild, all children or parent element, but there is no method for cleaning html code inside the node.

Root cause: The main issue was that we are replace only the first child, which is one of many inside the node.

Issue: #351

Video after the fix:

Screen.Recording.2021-04-21.at.23.37.09.mov

@@ -14,7 +14,7 @@ export const TimelineSourceIcon = styled.div<{
transition: background-color 150ms ease-out;

:before {
content: "${({ kind }) => kind[0].toUpperCase()}";
content: "${({ kind }) => kind && kind[0].toUpperCase()}";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Was this causing an issue?

Looking at the type definition this condition is always truthy

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Every time I open a RN app with the electron urql devtools I see this error on the Events page. I can open a different issue if you can reproduce it as well.

image

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmmm - I don't think this is a bug here, it sounds like we're ending up with invalid data.

Are you able to try and catch the event that we're sourcing the kind prop from. A console log somewhere around here should help us work out why we're missing the needed property

Copy link
Collaborator

Choose a reason for hiding this comment

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

If you want, lets revert the fix here for now so we can get this PR merged and then make a separate PR/issue for this particular problem!

It's a good find either way 🚀 we need to be able to trust our types so lets find the source 🕵️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted, I will investigate more. Thanks @andyrichardson

Comment on lines +50 to +54
if (ref.hasChildNodes()) {
ref.innerHTML = "";
}

ref.appendChild(child);
Copy link
Collaborator

@andyrichardson andyrichardson Apr 23, 2021

Choose a reason for hiding this comment

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

So simple 😄 Love it!

I'll test it out in chrome now 👍

Edit: Works a charm 🚀

@ValGeorgiev ValGeorgiev merged commit c40aa4e into master Apr 23, 2021
@ValGeorgiev ValGeorgiev deleted the fix/code-block-clean branch April 23, 2021 13:35
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.

None yet

2 participants