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

Improve Mark formatting #1352

Merged
merged 14 commits into from Mar 21, 2024
Merged

Improve Mark formatting #1352

merged 14 commits into from Mar 21, 2024

Conversation

geriux
Copy link
Member

@geriux geriux commented Mar 14, 2022

Related PRs:

This PR fixes several issues related to the Mark formatting.

  • It completes the missing functionality implementation to fully support the Mark tag.
  • Adds 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.
  • Updates MarkFormatter to set the expected CSS attributes as well as adding a color param.
  • Adds 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 with adjustAttributesForMark.
  • It also fixes toggleMark by allowing passing a color 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.

@geriux geriux force-pushed the feature/mark-formatting-improvements branch from 35b3492 to d655451 Compare March 14, 2022 08:17
@geriux
Copy link
Member Author

geriux commented Mar 18, 2022

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! 🙇

@twstokes
Copy link
Contributor

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! 🙇

Hey @geriux! 👋 I'll be glad to. 👍

@twstokes twstokes self-requested a review March 18, 2022 13:57
@geriux
Copy link
Member Author

geriux commented Mar 18, 2022

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! 🙇

Hey @geriux! 👋 I'll be glad to. 👍

🎉 thank you!

Copy link
Contributor

@twstokes twstokes left a 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.

  1. When viewing the HTML output, I found that one of my <mark> elements had two color properties. [couldn't reproduce]

Two color properties

  1. The formatting bar retained the highlighted color even when it wasn't being applied. [reproducible]
Formatting.bar.MP4
  1. 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]

  2. 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

Aztec/Classes/TextKit/TextStorage.swift Show resolved Hide resolved
Aztec/Classes/TextKit/TextStorage.swift Outdated Show resolved Hide resolved
Aztec/Classes/TextKit/TextStorage.swift Outdated Show resolved Hide resolved
@twstokes
Copy link
Contributor

👋 Hey @geriux! Is there anything I can do to help with this PR or have we decided not to pursue it for now? Thanks!

@geriux
Copy link
Member Author

geriux commented Dec 15, 2022

👋 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 @twstokes Thank you for reaching out! Due to other priorities, I couldn't focus on addressing your feedback but I will go back to this so we can include this fix. 🙇

@twstokes
Copy link
Contributor

👋 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 @twstokes Thank you for reaching out! Due to other priorities, I couldn't focus on addressing your feedback but I will go back to this so we can include this fix. 🙇

Sounds good!

@geriux geriux changed the title Mark Formatting - Fix autocorrected strings and changing the caret position bug Improve Mark formatting Jan 19, 2024
@geriux
Copy link
Member Author

geriux commented Jan 24, 2024

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 🚀

@geriux geriux requested a review from twstokes January 24, 2024 11:32
@twstokes twstokes self-assigned this Feb 15, 2024
Copy link
Contributor

@twstokes twstokes left a 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.

Copy link
Contributor

@twstokes twstokes left a comment

Choose a reason for hiding this comment

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

Looks great! 🙇

@geriux geriux merged commit f6501a7 into develop Mar 21, 2024
5 checks passed
@geriux geriux deleted the feature/mark-formatting-improvements branch March 21, 2024 11:10
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

2 participants