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

Added missing <tbody> tags after <table> in mjml-hero, mjml-button and mjml-social components #2111

Merged
merged 7 commits into from Jun 11, 2021

Conversation

Pharmasolin
Copy link
Contributor

I noticed that mjml-hero, mjml-button and mjml-social components didn't have <tbody> and </tbody>, this might add confusion for developers who might want to use mj-html-attributes component and css child combinator >.

@Pharmasolin
Copy link
Contributor Author

I think I messed PR a bit, I found that README.md has Contribute link which points to nowhere, but there is CONTRIBUTING.md file in repository and I think it should point there?

Please let me know if this url change in readme is not needed, I will resubmit PR then. (I forget to create new branch in my fork for this PR)

@iRyusa
Copy link
Member

iRyusa commented Dec 22, 2020

This was a reference to the old 3.X branch here https://github.com/mjmlio/mjml/tree/3.3.x#contribute I think you can remove it too

@Pharmasolin
Copy link
Contributor Author

@iRyusa should I revert change in last commit?

@iRyusa
Copy link
Member

iRyusa commented Dec 22, 2020

You can juste remove the link and mention i'll squash it

@Pharmasolin
Copy link
Contributor Author

@iRyusa removed contribution reference in readme.

@kmcb777
Copy link
Collaborator

kmcb777 commented Dec 23, 2020

thanks for this PR 👍
i think this should wait for v5 to be merged because it could break existing templates that use css selectors

@Pharmasolin
Copy link
Contributor Author

@kmcb777 100% would love to see it in v5 (I will need to update some of my templates too).

@iRyusa
Copy link
Member

iRyusa commented Jun 10, 2021

Even if it breaks a bit the DOM I think we should merge this in 4.10 as it's way better for accessibility cc @Pharmasolin can you resolve the conflict so we get this merged ?

@Pharmasolin
Copy link
Contributor Author

@iRyusa resolved the conflict, but please check if everything is ok before merging this branch to master. For example it has change to readme.md as well.

@iRyusa
Copy link
Member

iRyusa commented Jun 11, 2021

Looks good to me this section no longer exist in the readme anyway :D

@iRyusa iRyusa merged commit d1879e8 into mjmlio:master Jun 11, 2021
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.

None yet

3 participants