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

Disable HTML formatting by default #1214

Merged
merged 7 commits into from
Mar 31, 2023
Merged

Disable HTML formatting by default #1214

merged 7 commits into from
Mar 31, 2023

Conversation

osbre
Copy link
Contributor

@osbre osbre commented Mar 29, 2023

Closes #1213

@oliverklee oliverklee added this to the 8.0.0 milestone Mar 29, 2023
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Looks very good to me, thanks!

Could you also please add a line to the changelog (newest on top, and using the PR ID)? Thanks!

@osbre osbre requested a review from oliverklee March 30, 2023 11:54
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Now that I think of it, this seems to really solve a problem/bug for you. I'd like to avoid this problem creeping back in. So would you be willing to add a small regression test (that fails without the change and passes with the change)?

CHANGELOG.md Outdated Show resolved Hide resolved
osbre and others added 2 commits March 30, 2023 16:14
Co-authored-by: Oliver Klee <github@oliverklee.de>
@osbre osbre requested a review from oliverklee March 30, 2023 17:50
Copy link
Contributor

@oliverklee oliverklee left a comment

Choose a reason for hiding this comment

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

Beautiful. Thanks for contributing this, @osbre! ❤️

@oliverklee
Copy link
Contributor

Oh, the PHP Sniffer is not happy. I'll fix this.

@osbre osbre requested a review from oliverklee March 31, 2023 10:55
@oliverklee oliverklee merged commit e12b483 into MyIntervals:main Mar 31, 2023
Copy link
Contributor

@JakeQZ JakeQZ left a comment

Choose a reason for hiding this comment

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

Sorry, I'm a bit late back to the party.

Good job both, particularly the thoughtful discussion about the rationale, motivation, whys and wherefores, etc. I feel confident that the chosen solution is best.

However, I've spotted an issue with the CHANGELOG change which will need to be addressed. I don't know if it's possible (or desirable) to make further changes in a PR which has already been approved and merged.

I've also added a couple of other comments which are somewhat neither here nor there, and nothing to worry about.

Thanks @osbre for contributing.

CHANGELOG.md Show resolved Hide resolved
nlemoine pushed a commit to nlemoine/emogrifier that referenced this pull request Sep 19, 2023
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.

Disable HTML formatting by default
3 participants