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.

Sign up for GitHub

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

Jump to bottom

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 •
edited

Closes #1213

@oliverklee oliverklee added this to the 8.0.0 milestone Mar 29, 2023
oliverklee
oliverklee requested changes Mar 30, 2023
View reviewed changes
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
oliverklee
oliverklee requested changes Mar 30, 2023
View reviewed changes
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
@osbre @oliverklee
Update CHANGELOG.md
02bdb02 
Co-authored-by: Oliver Klee <github@oliverklee.de>
@osbre
Add a test to make sure rendered HTML has the same formatting as the …
edc5520 
…original
@osbre osbre requested a review from oliverklee March 30, 2023 17:50
oliverklee
oliverklee approved these changes Mar 31, 2023
View reviewed changes
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
21 checks passed
JakeQZ
JakeQZ reviewed Apr 4, 2023
View reviewed changes
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
tests/Unit/HtmlProcessor/AbstractHtmlProcessorTest.php Show resolved Hide resolved
tests/Unit/HtmlProcessor/AbstractHtmlProcessorTest.php Show resolved Hide resolved
nlemoine pushed a commit to nlemoine/emogrifier that referenced this pull request Sep 19, 2023
@osbre @nlemoine
[TASK] Disable HTML formatting by default (MyIntervals#1214)
b2f6325
@mortenscheel mortenscheel mentioned this pull request Nov 1, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JakeQZ JakeQZ JakeQZ left review comments

@oliverklee oliverklee oliverklee approved these changes

Assignees

@osbre osbre

Labels
None yet
Projects
None yet
Milestone
8.0.0
Development

Successfully merging this pull request may close these issues.

Disable HTML formatting by default
3 participants
@osbre @oliverklee @JakeQZ