Added backgroundColorSpan support v3 #1041
Conversation
- Added CssBackgroundColorPlugin based on CssUnderlinePlugin - Added AztecBackgroundColorSpan - General changes to implement and use BackgroundColorSpan
…as plugin so that could be optional
…acilitate the use on recyclerview with multiple edittext for example
Just standardizing the background color.
This condition will not mark background spans with different colors. It means background with different specified color will not have the background button highlighted.
This reverts commit 810fe3d.
There was a problem hiding this comment.
@yuvalgnessin-qz, I've added some initial comments/questions and am planning to take a look through this further with some guidance from @planarvoid in the coming week. 🙇♀️
aztec/src/main/kotlin/org/wordpress/aztec/formatting/InlineFormatter.kt
Outdated
Show resolved
Hide resolved
| @@ -468,7 +468,15 @@ class AztecToolbar : FrameLayout, IAztecToolbar, OnMenuItemClickListener { | |||
| toolbarButtonPlugins.add(buttonPlugin) | |||
|
|
|||
| val button = findViewById<ToggleButton>(buttonPlugin.action.buttonId) | |||
| button.setOnClickListener { buttonPlugin.toggle() } | |||
| val isToolbarAction = ToolbarAction.values().contains(buttonPlugin.action) | |||
There was a problem hiding this comment.
@yuvalgnessin-qz, I noticed Cameron's original comment on this code here was unresolved: https://github.com/wordpress-mobile/AztecEditor-Android/pull/934/files#r571833320
Can you let me know if you addressed the issues raised in that comment somewhere, perhaps I'm missing some context?
There was a problem hiding this comment.
Yes, this was addressed by ab286ed
If you comment out aztec.addPlugin(BackgroundColorButton(visualEditor)) you can see that the BG color button disappears
|
@SiobhyB thanks for your comments; you were right about many of the merge artifacts and the test change should never have happened; I think it was working around a bug.
So I believe everything should be addressed, but it seems that your CI isn't running on my branch. Please let me know if there's anything else! |
There was a problem hiding this comment.
I'm going ahead to approve this PR based on the following:
- The functionality works as expected in my testing, plus it's evidently been used for a few years in at least one third-party project.
- I can't spot any obvious issues with the code, all seems logical and makes sense to me.
- The changes have already gone through multiple rounds of feedback in Added backgroundColorSpan support #911 and Added backgroundColorSpan support v2 #934. In the most recent feedback, we were close to approving, with the only blocker being that connected tests were failing. However, there have since been further changes and all the tests now pass.
That said, I'd appreciate if someone with more experience with Aztec's codebase than I can double-check the changes, in case I'm missing any issues. @planarvoid has graciously offered to give the code a second check, and I'll hold off from merging the changes until he's able to do that. 🙇♀️
@yuvalgnessin-qz, thanks again for your persistence with this PR! It may be a couple of weeks until we have availability for the second review, but I'll be happy to merge these changes once we get that. 🎉
|
Thank you @SiobhyB and @planarvoid. Please do try to get to this as soon as you can so that we can finally put it behind us! |
|
@SiobhyB @planarvoid Any update on this? Sorry to be a bother, but I was hopeful that the 3rd time will be the charm and that we can finally get this merged upstream after my most recent changes. Unfortunately my branch is once again out of date with trunk. I'd be happy to update it one final time if you can dedicate the effort required on your end to get it merged, but otherwise I cannot promise to continue maintaining this PR. Thank you! |
|
@yuvalgnessin-qz, this PR's still on our radar, though I understand if you're not available to keep checks on it. The PR itself isn't currently showing any conflicts with |
|
@SiobhyB thanks for the response! I'm going to take this off my queue for now, but please ping me when you want to take next steps and hopefully I can jump back in. Thanks! |
|
As an update: @planarvoid is currently on leave, so won't be able to look at this any time in the coming months. However, in our brief discussion, he did note that he thinks this PR should be ready to be merged given all the factors outlined here. With all the previous reviews and the fact this works in my own testing too, I'm going to go ahead to merge this. @yuvalgnessin-qz, thank you again for all your time and work on this! |
|
That's great news! Thanks @SiobhyB 🎉 If you don't mind, please let me know when you release the next version of Thank you! |
|
@yuvalgnessin-qz, 👋 , just in case you missed it, the |
This PR is meant to replace #934 and #911
It contains the same changes, but it's been updated from trunk.
Feature
Test
Review
@SiobhyB