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: Better element ID naming for disambiguation (fixes #74) #77

Merged
merged 2 commits into from
Jun 24, 2024

Conversation

ngdangtu-vn
Copy link
Contributor

Summary

Suggest a new name for template ID in simple-template example.

Related issues

Fixes #74

Related content pull request

Later...

@ngdangtu-vn
Copy link
Contributor Author

ngdangtu-vn commented Mar 21, 2024

@bsmth I would like a feedback about the name. I try to go as simple as possible. It shouldn't change to much but still distinguishable.

@bsmth bsmth self-requested a review April 4, 2024 09:00
@bsmth
Copy link
Member

bsmth commented Apr 4, 2024

Hi there, sorry I forgot to reply. I've added myself as a reviewer so I don't forget.

simple-template/index.html Outdated Show resolved Hide resolved
@@ -31,4 +33,4 @@ <h1>Simple template</h1>
</my-paragraph>

</body>
</html>
</html>
Copy link
Member

Choose a reason for hiding this comment

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

(minor one, it's flagged here as no newline at end of file)

simple-template/main.js Outdated Show resolved Hide resolved
@bsmth bsmth changed the title fix #74 fix: Better element ID naming for disambiguation (fixes #74) Apr 4, 2024
@bsmth bsmth self-requested a review April 8, 2024 09:00
@bsmth
Copy link
Member

bsmth commented May 2, 2024

Hi @ngdangtu-vn what do you think about these changes? 😄 Some comments for you to have a look at. Thanks!

@bsmth bsmth added the awaiting response Waiting on the author to address comments or other feedback label May 2, 2024
@ngdangtu-vn
Copy link
Contributor Author

I'm so sorry, I thought I left feedback already. Turn out I didn't :))

I personally don't mind to use shorter name. However, because this example will be used across the MDN doc as small pieces (not a whole example), I decided to go with more semantic than short words.

What do you think?

@bsmth
Copy link
Member

bsmth commented May 6, 2024

I'm so sorry, I thought I left feedback already. Turn out I didn't :))

Not a problem!

I personally don't mind to use shorter name. However, because this example will be used across the MDN doc as small pieces (not a whole example), I decided to go with more semantic than short words.

What do you think?

Yes, I see what you mean. What about custom-paragraph (to get rid of my in this case)?

@ngdangtu-vn
Copy link
Contributor Author

What about custom-paragraph (to get rid of my in this case)?

Yup, that's better.

simple-template/index.html Outdated Show resolved Hide resolved
@bsmth
Copy link
Member

bsmth commented May 7, 2024

What about custom-paragraph (to get rid of my in this case)?

Yup, that's better.

Nice, I've edited the suggestions if you want to have a look 👀

Co-authored-by: Brian Thomas Smith <[email protected]>
Copy link
Member

@bsmth bsmth left a comment

Choose a reason for hiding this comment

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

Thanks a lot! This looks good to me, I think we can merge and create a PR in mdn/content to update the code and prose there to reflect the naming change!

@bsmth bsmth added needs content update Needs update to the content to support this change. and removed awaiting response Waiting on the author to address comments or other feedback labels May 14, 2024
@bsmth
Copy link
Member

bsmth commented May 14, 2024

Actually I'll hold off on merging until we're happy with the content PR. The pages that need an update are in the linked issue, as far as I can see.

@ngdangtu-vn
Copy link
Contributor Author

@bsmth So for now, we agree with the new name for the template tag ID and starting to update the content on other MDN pages. Could you confirm if I understand correctly? If so, I'll start on update the tag ID on other MDN pages :)

@bsmth
Copy link
Member

bsmth commented May 14, 2024

@bsmth So for now, we agree with the new name for the template tag ID and starting to update the content on other MDN pages. Could you confirm if I understand correctly? If so, I'll start on update the tag ID on other MDN pages :)

Yes! That sounds good, when you are ready with changes, you can tag me in the PR and I will happily review! Thank you

@bsmth
Copy link
Member

bsmth commented Jun 24, 2024

Let's get it merged 🚢

@bsmth bsmth merged commit d8f4090 into mdn:main Jun 24, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs content update Needs update to the content to support this change.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve readability for 'simple-template' example
2 participants