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

Usage of DOM to create HTML #211

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

An0n3m0us
Copy link
Contributor

@An0n3m0us An0n3m0us commented Mar 5, 2021

Related to #190
Conflicting changes with #201

The aim of this PR is simply to convert all of the strings used to create HTML code into JS DOM.
I have opened this as a draft PR for discussion and clarification purposes.

  • Convert all HTML strings to DOM
  • Check that nothing has been lost in conversion
  • Clean parts of the code that have been added

@An0n3m0us An0n3m0us marked this pull request as draft March 5, 2021 10:27
@An0n3m0us
Copy link
Contributor Author

@JettBurns14 #201 is kinda bothering me as its code needs to be converted too.
Shall I just leave it for now or implement it into this PR?

@JettBurns14
Copy link
Member

I like this, but question: can you check some of our other code and see which way we created HTML there? JS DOM or strings? I can't remember the overall method we used, though I can tell we have a mix in this area.

I like strings, since it's more compact and visual, but JS DOM may be more "correct". TBH we should technically use a lib like React DOM, designed exactly for this. Not sure if we can import just that part of React, otherwise if it's too large we should use something else. But yeah, let me know how we've mostly done it so far.

@An0n3m0us
Copy link
Contributor Author

I think I don't quite understand what you mean by other code.

I went through most of the files and I changed the HTML strings but left the JS DOM parts of the code that already existed.
But yeah, it seems there was a mix of JS DOM and HTML strings used.

For instance, in buttons.ts, DOM was used to create a button, but <span> was use in the innerHTML code for the button.
https://github.com/ka-extension/ka-extension-ts/pull/211/files#diff-93a2269acab5a1e96b066b36204a2085f266ac93c1fed5affbe566c7bdc3c345R72

There are still one or two files left that I need to do the conversions for though.

@JettBurns14
Copy link
Member

JettBurns14 commented Mar 6, 2021

I just meant check other code in src/, but it's fine, looks like we mostly use JS DOM with string literals mixed in when we got lazy haha.

Yeah, go ahead and switch the string literals with HTML tags inside to JS DOM for all files. There are obviously places we don't need to convert the literals, like here, since they don't contain HTML. Be sure to double check all the elements you're re-writing, I believe you missed something earlier, which I'll point out.

@MatthiasPortzel
Copy link
Contributor

You shouldn’t need to use innerHTML at all. If it’s not HTML content, don’t worry about how to escape it, just set .textContent instead.

@MatthiasPortzel
Copy link
Contributor

MatthiasPortzel commented Mar 7, 2021

I don’t know what @JettBurns14 is talking about, I don’t know what escapeHTML is.

@JettBurns14
Copy link
Member

JettBurns14 commented Mar 7, 2021

Good point @matthiassaihttam, textContent would work better here. No need to escape that data then.

escapeHTML is imported from ./util/text-util here. Ethan built most of the utilities.

@JettBurns14 JettBurns14 added the Code improvement For issues that relate to code, not functionality label Mar 7, 2021
@An0n3m0us An0n3m0us marked this pull request as ready for review March 20, 2021 08:11
@An0n3m0us
Copy link
Contributor Author

An0n3m0us commented Mar 20, 2021

I have now completed the conversion of all of the HTML strings that I have found, so I've removed the draft status on the PR.
If someone could do a once-over on the code, that would be great.

@An0n3m0us An0n3m0us requested a review from JettBurns14 March 24, 2021 20:24
@MatthiasPortzel
Copy link
Contributor

This looked mostly good when I tested it. There were issues that arose from not properly rendering HTML in the notifications. I'll need to go through the code at some point and figure out where that issue is, and how Firefox would have us fix it.

@An0n3m0us
Copy link
Contributor Author

Hmm, I'm pretty sure I checked everything when I last updated this PR though.
But as with my other PRs, I'm planning to get back to this one sometime in mid May.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code improvement For issues that relate to code, not functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants