-
Notifications
You must be signed in to change notification settings - Fork 147
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
Improve Mark formatting #1352
Improve Mark formatting #1352
Conversation
35b3492
to
d655451
Compare
Hey @twstokes 👋 I was wondering if you could help me out with a review for this PR since part of it uses the same code you added for the Heading fix for the highlighting text feature. No worries if you can't, let me know either way. Thanks! 🙇 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👋 @geriux! I had a few questions in the PR, but otherwise the tests from the Gutenberg PR passed. 👍
I did notice a few things that looked like bugs that I wanted to share. They may not be due to this PR and you may already be aware of them. Unfortunately, only two were reproducible.
- When viewing the HTML output, I found that one of my
<mark>
elements had twocolor
properties. [couldn't reproduce]
- The formatting bar retained the highlighted color even when it wasn't being applied. [reproducible]
Formatting.bar.MP4
-
Starting with an empty paragraph block, choosing a highlight color, and typing resulted in no highlighted text. It was as if I'd never chosen the color. I didn't get a chance to grab a video of this one. [couldn't reproduce]
-
Text color didn't match the custom color chosen. I confirmed this also happens in the latest TestFlight version 19.4.0.3. This was a simple site running the Blank Canvas theme. It only happened with custom colors chosen at the bottom of the picker, not the custom color picker in the "Theme" section. [reproducible]
Custom.color.MP4
👋 Hey @geriux! Is there anything I can do to help with this PR or have we decided not to pursue it for now? Thanks! |
Hey there @twstokes 👋 I'm sorry this took so long to move forward but I've introduced new changes that should fix the issues you mentioned above 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work @geriux! 🚀 This makes sense to me, and I added two comments that are optional nits.
I ran through a couple tests, but mostly relied on David and Carlos' results in this PR. If you think I should do all of the manual tests as well I'd be happy to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great! 🙇
Related PRs:
This PR fixes several issues related to the Mark formatting.
MarkStringAttributeConverter
which will handle toggling the format as well as returning the expected HTML format to Gutenberg, by removing the extra spaces. This was causing issues like history changes or showing the active style indicator when it shouldn't.MarkFormatter
to set the expected CSS attributes as well as adding acolor
param.preprocessMarkForInsertion
that will handle cases like auto-corrected strings, it will check if the new auto-corrected words need to have the Mark representation along withadjustAttributesForMark
.toggleMark
by allowing passing acolor
param and to be able to change colors while editing a Mark formatting or resetting the existing format.To test
Check this PR description under the
Test
section. Where there are builds linked as well.