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

Replace usages of innerHTML with innerText or by appending elements #125

Merged
merged 1 commit into from
Mar 21, 2021

Conversation

vezwork
Copy link
Member

@vezwork vezwork commented Mar 21, 2021

Fixes part 4 (Warnings: Use of .innerHTML with dynamic content.) of #65.

Changes

  • Replace usages of element.innerHTML either with:
    • .innerText (when the element only contains text) or
    • using new HTML helper functions to programmatically create and insert html elements into the element (when the element contains other elements). These HTML helper function are located at the bottom of popup.js.
  • Renames getSentAssetsMapAsString to getSentAssetsMapAsEls and moves it from main.js to popup.js (since it now uses the HTML helper functions).
  • Add css styling to a and p elements in top site detail to make up for styling previously added by .innerHTML.

Testing Notes

  • Test links and text in Chrome, Edge, and Firefox.
  • No functionality or appearances should have changed.
  • Please ensure text and links display correctly in the following areas and circumstances:
    • when no sites have been visited, on the main page,
    • when monetized sites have been visited without a provider, on the main page,
    • when monetized sites have been visited with a provider, on the main page,
    • top site detail without a provider, and
    • top site detail with a provider.

@vezwork vezwork requested a review from sharon-wang March 21, 2021 03:43
@vezwork
Copy link
Member Author

vezwork commented Mar 21, 2021

I have tested locally without a provider, and it is working as expected✅.

@vezwork
Copy link
Member Author

vezwork commented Mar 21, 2021

I have tested locally with Coil, and it is working as expected✅.

@vezwork vezwork changed the title Replace usages of innerHTML with innerText or by appending elements. Replace usages of innerHTML with innerText or by appending elements Mar 21, 2021
Copy link
Member

@sharon-wang sharon-wang left a comment

Choose a reason for hiding this comment

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

Just a bunch of nits, otherwise looks good!

Going to test on some browsers now.

src/popup/popup.js Outdated Show resolved Hide resolved
src/popup/popup.js Outdated Show resolved Hide resolved
src/popup/popup.js Outdated Show resolved Hide resolved
src/popup/popup.js Outdated Show resolved Hide resolved
src/popup/popup.js Outdated Show resolved Hide resolved
Comment on lines 376 to 379
// return `<a href="${origin}" style="color: black; text-decoration: underline;">${origin}</a><br><br>
// ${timeSpentString}
// ${visitCountString}
// ${paymentString}`;
Copy link
Member

Choose a reason for hiding this comment

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

pls yeet this

src/popup/popup.js Outdated Show resolved Hide resolved
src/popup/popup.js Outdated Show resolved Hide resolved
src/popup/popup.js Outdated Show resolved Hide resolved
src/popup/popup.js Outdated Show resolved Hide resolved
@sharon-wang
Copy link
Member

sharon-wang commented Mar 21, 2021

Looks good on Chrome, Brave, Edge. Will test on Firefox after your updates & rebase off master.

(testing without WM provider)

@vezwork vezwork force-pushed the removeInnerHTML branch 3 times, most recently from 77793a7 to 5e13af6 Compare March 21, 2021 06:48
@sharon-wang
Copy link
Member

Looks good on FF too. Ready for a squash and then merge :)

Moves getSentAssetsMapAsString to popup.js and make it return els

address comments - make use of backticks consistent
@vezwork vezwork merged commit 5f4838a into esse-dev:master Mar 21, 2021
Copy link
Member

@sharon-wang sharon-wang left a comment

Choose a reason for hiding this comment

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

This was merged without explicit "Approval", but this is approved.

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