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

Replaced innerHTML with innerText in scripts/main.js #39

Closed
wants to merge 1 commit into from

Conversation

FatihTheDev
Copy link

Description

Motivation

Additional details

Related issues and pull requests

@FatihTheDev FatihTheDev requested a review from a team as a code owner April 16, 2024 09:03
@FatihTheDev FatihTheDev requested review from pepelsbey and removed request for a team April 16, 2024 09:03
Copy link

It looks like this is your first pull request. 🎉
Thank you for your contribution! One of the project maintainers will triage
and assign the pull request for review. We appreciate your patience. To
safeguard the health of the project, please take a moment to read our
code of conduct.

@pepelsbey
Copy link
Member

Hey! Thank you for the suggestion. This demo is used in the JavaScript basics article that has already used textContent in the examples. I’d say it would be good to align the demo with the article.

By the way, there’s another innerHTML instance earlier in the script that would be good to replace, too.

As for the comment about using innerHTML, I think it belongs in the article, not the demo. However, since the article is for beginners, I’d leave it as simple as possible and suggest the safer option by default.

@FatihTheDev
Copy link
Author

By the way, there’s another innerHTML instance earlier in the script that would be good to replace, too.

I just realized that after pushing the changes. 😅

Thank you for the response!

@pepelsbey pepelsbey added the enhancement Improves an existing feature. label Apr 18, 2024
@github-actions github-actions bot added the idle Issues and pull requests with no activity for three months. label May 19, 2024
@pepelsbey
Copy link
Member

@FatihTheDev hey! Do you think we can finish this one? If you have time, of course. Otherwise, I’d close the PR.

@github-actions github-actions bot removed the idle Issues and pull requests with no activity for three months. label Aug 22, 2024
@pepelsbey
Copy link
Member

Closing for now. Let me know if you’d like to get back to it.

@pepelsbey pepelsbey closed this Sep 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Improves an existing feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants