Added backgroundColorSpan support v2 #934
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.
|
@jd-alexander since you seem to be the most active maintainer currently on this project, do you think you might be able to look at this pull request as well? I'm also hoping you could help me fix the issue with |
|
Since you reviewed the previous PR, could you take a look at this one as well @cameronvoell ? |
|
Thanks for taking a look @cameronvoell and @mchowning! Any ETA on when this might be reviewed? |
|
@yuvalgnessin-qz Thanks for your patience. I have this in my queue and should be able to get to it before the end of this week, and hopefully earlier. Looking forward to reviewing the updates. 🙇 |
|
Hi again @cameronvoell - do you still think you might be able to get to this today, or will it have to wait until next week? No worries either way; I'm sure you're very busy! |
| override val textFormats: Set<ITextFormat> = setOf()) | ||
| : IToolbarAction { | ||
|
|
||
| BACKGROUND( |
There was a problem hiding this comment.
Hi @yuvalgnessin-qz @lvcasasanta, see point 3 in this comment: #911 (review). With the new background button, I think it makes sense to have it be easy for each project using Aztec to enable or disable the background button based off whether it is added as a plugin. @felipevaladares addressed this concern with this commit in the previous PR a95d50e, but you are reversing that by re-adding the background button to the default Toolbar section here.
I see this change was part of your commit for fixing the button active state issue. Can you fix that issue while maintaining our ability to not add the background button if the plugin is not added to our aztec instance? For reference you can check out the behavior of the Page and More buttons. If you comment out the lines where they are added, they no longer appear in the toolbar.
Let me know if that makes sense. Thanks!
Hi @yuvalgnessin-qz See my comment here: #934 (comment) My testing looked good for the other concerns so far, there is just that one issue in that comment above that needs to be addressed. I'll test some more this week to see if there are any other concerns. |
|
Thanks for the feedback @cameronvoell! I'll try to find time to address your concern today. FWIW, as of last week we have already shipped an app to production that uses the code in this branch, and so far it seems stable! |
|
@cameronvoell I just pushed a commit with the changes you requested. Now, the background color will not appear unless it is added as a plugin. In order to make this work, I had to modify the logic in Check out the last commit (ab286ed) for the changes in question. Hopefully now we are close to getting this merged! |
|
Hi again @cameronvoell! Any updates? |
|
Hello again @cameronvoell! Do you think there's any chance of having this merged upstream at some point? Or are you and your team too busy right now? If so, I can close the PR - let me know. |
|
Hi @yuvalgnessin-qz Sorry I was MIA for a bit. The changes you made look good. Unfortunately I'm seeing some failing end to end or "connected" tests. CircleCI is failing on this PR because it's coming from a fork, so that's fine, but you can see on this other branch I made that when connected tests are run here, we are seeing some failures: https://app.circleci.com/pipelines/github/wordpress-mobile/AztecEditor-Android/464/workflows/69fa00ae-551c-4e6d-9b06-1043f15945dc/jobs/1329 You can run those tests locally using As far as I can tell the failing tests are all crashing at this line here: Apologize for the gap in the reviews, I can check more regularly moving forward on this. |
| val isToolbarAction = ToolbarAction.values().contains(buttonPlugin.action) | ||
| button.setOnClickListener { | ||
| if (isToolbarAction) { | ||
| onToolbarAction(buttonPlugin.action) |
There was a problem hiding this comment.
Hi @cameronvoell , thanks for taking another look! Based on the stacktrace it looks like the errors are coming from this change right here. Before these changes, I believe Nevermind, I see that line 427 has onToolbarAction would never have been called from a button click listener.button?.setOnClickListener { onToolbarAction(toolbarAction) }
I will try to find time to look into this deeper today. It's been a while since I was originally working on this and have since moved on to other projects, so I'm not sure how much time I'll be able to dedicate to it now. But I'll do my best
| action != ToolbarAction.ELLIPSIS_EXPAND) { | ||
| val view = findViewById<ToggleButton>(action.buttonId) | ||
| if (view.isChecked) actions.add(action) | ||
| if (view?.isChecked == true) actions.add(action) |
There was a problem hiding this comment.
Hi @cameronvoell, this change should fix the IllegalStateException failures on CI. At first I debated whether this was the right solution, but I think it makes sense. Before this change, we were guaranteed that all toolbar actions were always present. But now that background color is an optional plugin, for the first time ever it's possible that one of the actions in ToolbarAction.values() is not present.
An alternative way to fix these tests would've been to move the following out of the if (!isRunningTest) branch:
aztec.addPlugin(CssBackgroundColorPlugin())
aztec.addPlugin(BackgroundColorButton(visualEditor))TBH I'm not exactly sure why we chose to put that inside if (!isRunningTest) in the first place, but I felt that this was a better solution overall to prevent crashes from users of the library that don't add the plugin.
Unfortunately this change alone does not resolve all of the espresso tests though. Some tests were fully fixed by this, but some of them are now failing for different reasons. One such example is FormattingHistoryTests.testAddBoldUndoRedo(), which now fails like this:
Expected: EditText matches <strong>Testing</strong>
Got: "SourceViewEditText{ ... text=<strong><b>T</b></strong><b>esting</b> ... }"
I have no idea why the changes in this PR would cause the <b> tags to be added, or why they'd be cutting of the "T" in "Testing". It may be due to changes that occurred before I got involved. As someone with more context on this project, do you have any ideas what might be causing this?
Thanks very much!
|
@yuvalgnessin-qz Could you please share your source code gradle link so that i can locally execute the color span functionality with my application. i have been using the aztec toolbar in my application. |
|
@imthiaz you can view our fork and its releases here and here, and use Something like this: You are free to use it, but I cannot make any guarantees regarding how long we will be maintaining this fork and whether we will be updating it from the upstream project, nor can I make any guarantees regarding what versioning scheme we may use if we do make changes in the future. For now, it is not in active development. The best long-term approach for maintainability would be for Wordpress to merge this PR upstream so that we can both depend on the original dependency, rather than on the fork. |
|
Sure thanks for your quick response and I wil use the mentioned one and
test it with my application.
I wish to have the PR merged asap👍👍
…On Thu, 8 Apr, 2021, 12:22 am Yuval Gnessin, ***@***.***> wrote:
@imthiaz <https://github.com/imthiaz> you can view our fork and its
releases here
<https://github.com/quizlet/AztecEditor-Android/tree/feature/background-color-span-support>
and here <https://github.com/quizlet/AztecEditor-Android/releases>, and
use jitpack to package it through github the same way you would the
wordpress artifact.
Something like this:
repositories {
maven { url 'https://jitpack.io' } // For projects hosted on github
}
...
dependencies {
implementation("com.github.quizlet:AztecEditor-Android:background-color-support-v2.1")
}
You are free to use it, but I cannot make any guarantees regarding how
long we will be maintaining this fork and whether we will be updating it
from the upstream project, nor can I make any guarantees regarding what
versioning scheme we may use if we do make changes in the future. For now,
it is not in active development. The best long-term approach for
maintainability would be for Wordpress to merge this PR upstream so that we
can both depend on the original dependency, rather than on the fork.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#934 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4HS4OPZIXBLKYBI2EWM4LTHSSWXANCNFSM4WPIIBIA>
.
|
|
@imthiaz you can set the background color to whatever you want using |
Ok thanks for the update. @yuvalgnessin-qz Do we also support font color change from your version of aztec, as i could see in the above mentioned first AztecDEMO screenshot some text in green color - i.e Italic (Red), Underline (Green with underline.) Can you let me know how to input such green colored words using aztec if possible. Initially i asssumed aztec android version does not support font color options |
|
@imthiaz I am not sure if Aztec supports text color changing, because that's not something I use it for. It looks like the underline text you asked about in the demo is hardcoded with HTML like this: This pull request is for background color support only. I am not a maintainer of this library and have no intention of adding any other features to it at this time. If you have any questions that are not related to background color, please open an Issue on the github project. And if there are any missing features that you'd like to add, feel free to open a pull request! |
|
Sure understood 👍
…On Sat, 10 Apr, 2021, 3:11 am Yuval Gnessin, ***@***.***> wrote:
@imthiaz <https://github.com/imthiaz> I am not sure if Aztec supports
text color changing, because that's not something I use it for. It looks
like the underline text you asked about in the demo is hardcoded with HTML
like this: "<u style=\"color:lime\">Underline</u><br>". You can see that
here
<https://github.com/wordpress-mobile/AztecEditor-Android/pull/934/files#diff-f560a91308e1ade3461de1cb5c543283d28801832ecf0a1475be13ce4eae23ecR93>.
I don't know if Aztec supports changing it via toolbar.
This pull request is for background color support only. I am not a
maintainer of this library and have no intention of adding any other
features to it at this time. If you have any questions that are not related
to background color, please open an Issue on the github project. And if
there are any missing features that you'd like to add, feel free to open a
pull request!
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#934 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AA4HS4JNLJLF4KMT5WOZU3DTH5YAHANCNFSM4WPIIBIA>
.
|
|
Hello @oguzkocer @fluiddot @planarvoid @khaykov ! I understand that between the time this PR was opened and approximately October 2021, this project was inconsistently maintained. However, I'm happy to see that for the past year or so there have been regular releases. If I update this branch against trunk, would you all consider merging it upstream, and if necessary, help me get it over the finish line? Thank you! |
|
Hi there @yuvalgnessin-qz 👋 I am sorry about the late reply. I was on an extended leave at the time of your ping. I brought this up internally and @SiobhyB graciously volunteered with the review process. I think she is planning to leave a reply as well. |
|
@oguzkocer thanks for the reply, and welcome back. I did in fact update my fork from trunk back in January when I wrote the comment, but the changes are in another branch. I would be happy to either update this branch with those changes or open a new PR from the updated branch. @SiobhyB if I do that, will you be able to help with next steps to get this merged upstream? Thanks! |
|
👋 @yuvalgnessin-qz, thanks for your persistence and patience with these changes! I'd be happy to work towards getting this merged with you. Merging the latest changes from |
|
@SiobhyB sounds good. I'll let you know when that's done, then hand it off to you. Thanks! |
|
@yuvalgnessin-qz, thank you! I'll go ahead to close this one. |
This PR is meant to replace #911 since @felipevaladares was no longer available to complete the requested changes.
This branch is forked from his and addresses all remaining comments from @cameronvoell from that PR, including fixing the bug that keeps the background button highlighted.
All changes since #911 are implemented by @lvcasasanta
Feature
Test
Review
@cameronvoell