jonathanbossenger / stand-with-ukraine

Stand with Ukraine
https://github.com/jonathanbossenger/stand-with-ukraine/archive/refs/tags/1.0.0.zip
1 stars 4 forks source link

js: fixes Argument type ChildNode is not assignable to parameter type Node #8

Closed erikyo closed 2 years ago

erikyo commented 2 years ago

My contribution to a great cause, I'm just doing it for doing a little bit of my own part.

jonathanbossenger commented 2 years ago

Thanks @erikyo

@bueltge as the person who submitted #3, could I ask you to review this PR first, as a sanity check.

jonathanbossenger commented 2 years ago

@rosswintle would you mind taking a peek at this PR as well, and leaving your thoughts on @erikyo's comments/suggestions.

rosswintle commented 2 years ago

Thanks for this contribution. Every suggestion is appreciated!

I'm not sure what this fixes though. Can you explain where you were seeing the error? What browser are you using? What theme is it showing in? If I can reproduce the error I can see if this change fixes something.

Personally, I prefer the original style, but it's @jonathanbossenger's plugin so I'll let him decide on the design. I like that you made the whole banner width clickable though.

There's a comment about IE11 compatibility but as far as I can tell the changes don't help with that. The code still uses let/const, and prepend has been added. I'd love for this to be IE11 compatibile but it looks like more changes are needed to make that happen.

Happy to merge if the design changes are OK. But I think more work is needed.

jonathanbossenger commented 2 years ago

Personally, I prefer the original style, but it's @jonathanbossenger's plugin so I'll let him decide on the design. I like that you made the whole banner width clickable though.

Thanks @rosswintle I'll review the style later today, and leave my comments here.

erikyo commented 2 years ago

I'm not sure what this fixes though. Can you explain where you were seeing the error? What browser are you using? What theme is it showing in? If I can reproduce the error I can see if this change fixes something.

The reason is that document.body is a Node and firstchild use in its constructor ChildNode (type ChildNode is not assignable to type Node). Detected using typescript console, because it does not really generate an error (ChildNode extends Node) but i guess isn't 100% correct https://microsoft.github.io/PowerBI-JavaScript/interfaces/_node_modules_typedoc_node_modules_typescript_lib_lib_dom_d_.childnode.html#firstchild https://dom.spec.whatwg.org/#childnode

About the style I noticed that it could be reduced and one property did not work (text-combine: #0057B8;) that's the main reason for the style changes. And ok, then I wanted to put my own spin on it, and made the banner look like the flag (and I was thought it looked awesome, but I understand it might be my own thing, degustibus non disputandum est - what you like isn't questionable :P)

image image

jonathanbossenger commented 2 years ago

@erikyo I see there is a conflict with the stand_with_ukraine.js file. This is probably because I updated it after you created your PR. could I ask you to update your PR to resolve the conflict, then I can review it, thanks.

erikyo commented 2 years ago

no problem for that i can rebase the PR 😉

erikyo commented 2 years ago

then if you want I'll also pr for the style, but it was really wrong to include it in this one.

jonathanbossenger commented 2 years ago

then if you want I'll also pr for the style, but it was really wrong to include it in this one.

That's no problem, you are welcome to fork the repo, and change the style to suit your preference.

jonathanbossenger commented 2 years ago

@erikyo for some reason, when I test the code from your branch, the font in the banner changes.

Code from the main branch: WordPress-Local-1

Code from your branch WordPress-Local-2

Are you able to test this, and check why it might be the case?

erikyo commented 2 years ago

Yes you were right and the banners were not in the same place... and that's why they weren't look like the same. Fixed and now they should be perfectly identical. To test you can do this:

document.addEventListener('DOMContentLoaded', function() {
    let body = document.getElementsByTagName('body')[0];
    let div  = document.createElement('div');
    div.id = 'stand_with_ukraine_overlay';
    div.innerHTML = '<a title="' + swu_options.text + '" href="' + swu_options.url + '">' + swu_options.hashtag + '</a>';
    document.body.insertBefore(div, body.firstChild);

    var divNew  = document.createElement('div');
    divNew.id = 'stand_with_ukraine_overlay';
    divNew.innerHTML = '<a title="' + swu_options.text + '" href="' + swu_options.url + '">' + swu_options.hashtag + '</a>';
    document.body.insertAdjacentElement('afterbegin', divNew);
});