Added backgroundColorSpan support #911

felipevaladares · 2020-05-26T21:45:37Z

Added CssBackgroundColorPlugin based on CssUnderlinePlugin
Added AztecBackgroundColorSpan
General changes to implement and use BackgroundColorSpan

Test

Run the app
Select some text
Click on the new button that is located between Bold and Quote

Review

- Added CssBackgroundColorPlugin based on CssUnderlinePlugin - Added AztecBackgroundColorSpan - General changes to implement and use BackgroundColorSpan

duyvt88 · 2020-05-31T16:02:24Z

@felipevaladares Hi, can we change the text color of the selected text?
Many thanks!

felipevaladares · 2020-05-31T21:35:56Z

@felipevaladares Hi, can we change the text color of the selected text?
Many thanks!

This is to change the background color, not the text color, but you can use both

felipevaladares · 2020-05-31T21:37:43Z

the idea is to use in cases like the image on this #910

duyvt88 · 2020-06-01T14:40:38Z

@felipevaladares Hi, can we change the text color of the selected text?
Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

felipevaladares · 2020-06-01T15:06:24Z

@felipevaladares Hi, can we change the text color of the selected text?
Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

I'm not sure if you will be able to do that using my code, you should probably create a new Span and Plugin based on my changes, i believe it will be very similar

duyvt88 · 2020-06-06T03:51:26Z

@felipevaladares Hi, can we change the text color of the selected text?
Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

I'm not sure if you will be able to do that using my code, you should probably create a new Span and Plugin based on my changes, i believe it will be very similar

I have done it. Thanks for your support.

Do you think we can change the font size of the selected text?
Thanks,

felipevaladares · 2020-06-08T14:31:19Z

@felipevaladares Hi, can we change the text color of the selected text?
Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

I'm not sure if you will be able to do that using my code, you should probably create a new Span and Plugin based on my changes, i believe it will be very similar

I have done it. Thanks for your support.

Do you think we can change the font size of the selected text?
Thanks,

Nice, i'm curious to see your code, have you opened a PR?

felipevaladares · 2020-06-08T14:33:47Z

@koke @maxme @bummytime @shiki @Tug could you guys please review this PR?

cameronvoell

Nice work here @felipevaladares. Changes seem on the whole consistent with the existing code base, so nice job with that. However, I did notice a few minor issues that would be good to fix before merging this in.

While testing your PR, I noticed an existing issue that becomes more noticeable with your changes: Format text then remove format removes color style #914 . If you toggle text with background color bold or italic etc., and then toggle back off, the background color disappears unexpectedly. Ideally this is fixed before merging.
In the past we had other issues where certain text patterns did not show a visual for highlighting, or show a cursor when it is placed in the text. I think we are seeing similar issues with text with a backgroundColorSpan, highlighting shows no color change, and the cursor is not visible. Perhaps previous fixes for these types of issues will provide hints on how to fix that. (Issue/219 code highlight color #244 , Fixin visibility of cursor in quote #249)
Perhaps for at least this first iteration we can add Background Color button to the Plugin buttons section of the toolbar, so that it is easy to not include the button if we do not add the new CssBackgroundColorPlugin. This way existing apps using Aztec will not have the new button show up unless they want to use it. (For example, notice if you comment this line

AztecEditor-Android/app/src/main/kotlin/org/wordpress/aztec/demo/MainActivity.kt

Line 443 in a1386be

.addPlugin(MoreToolbarButton(visualEditor))

the More Button will disappear from the toolbar, but this is not the case if we remove your aztec.addPlugin(CssBackgroundColorPlugin()) because it is in the main layout sections of simple and advanced toolbars instead of the plugin section.)

Thanks again for the changes here.

felipevaladares · 2020-06-12T16:19:57Z

Nice work here @felipevaladares. Changes seem on the whole consistent with the existing code base, so nice job with that. However, I did notice a few minor issues that would be good to fix before merging this in.

1. While testing your PR, I noticed an existing issue that becomes more noticeable with your changes: #914 . If you toggle text with background color bold or italic etc., and then toggle back off, the background color disappears unexpectedly. Ideally this is fixed before merging.

2. In the past we had other issues where certain text patterns did not show a visual for highlighting, or show a cursor when it is placed in the text. I think we are seeing similar issues with text with a backgroundColorSpan, highlighting shows no color change, and the cursor is not visible. Perhaps previous fixes for these types of issues will provide hints on how to fix that. (#244 , #249)

3. Perhaps for at least this first iteration we can add Background Color button to the Plugin buttons section of the toolbar, so that it is easy to not include the button if we do not add the new CssBackgroundColorPlugin. This way existing apps using Aztec will not have the new button show up unless they want to use it. (For example, notice if you comment this line https://github.com/wordpress-mobile/AztecEditor-Android/blob/a1386beed8d8215fe8ab79c501c1ec237ae8a785/app/src/main/kotlin/org/wordpress/aztec/demo/MainActivity.kt#L443
    the More Button will disappear from the toolbar, but this is not the case if we remove your `aztec.addPlugin(CssBackgroundColorPlugin())` because it is in the main layout sections of simple and advanced toolbars instead of the plugin section.)

Thanks again for the changes here.

Hi @cameronvoell thanks for reviewing, i'll try to handle all the issues and improvements as soon as possible :)

duyvt88 · 2020-06-18T16:33:23Z

@felipevaladares Hi, can we change the text color of the selected text?
Many thanks!

This is to change the background color, not the text color, but you can use both

Thanks for your reply. I'm trying to customize your code to change the text color.

I'm not sure if you will be able to do that using my code, you should probably create a new Span and Plugin based on my changes, i believe it will be very similar

I have done it. Thanks for your support.

Do you think we can change the font size of the selected text?
Thanks,

Nice, i'm curious to see your code, have you opened a PR?

Hi @felipevaladares,
Sorry for replying late. Please check my code here https://github.com/duyvt88/AztecEditorCustom.

Thank you very much!

…ground colors

…lic action

…as plugin so that could be optional

arctouch-felipevaladares · 2020-07-03T18:07:47Z

Nice work here @felipevaladares. Changes seem on the whole consistent with the existing code base, so nice job with that. However, I did notice a few minor issues that would be good to fix before merging this in.

1. While testing your PR, I noticed an existing issue that becomes more noticeable with your changes: #914 . If you toggle text with background color bold or italic etc., and then toggle back off, the background color disappears unexpectedly. Ideally this is fixed before merging.

2. In the past we had other issues where certain text patterns did not show a visual for highlighting, or show a cursor when it is placed in the text. I think we are seeing similar issues with text with a backgroundColorSpan, highlighting shows no color change, and the cursor is not visible. Perhaps previous fixes for these types of issues will provide hints on how to fix that. (#244 , #249)

3. Perhaps for at least this first iteration we can add Background Color button to the Plugin buttons section of the toolbar, so that it is easy to not include the button if we do not add the new CssBackgroundColorPlugin. This way existing apps using Aztec will not have the new button show up unless they want to use it. (For example, notice if you comment this line https://github.com/wordpress-mobile/AztecEditor-Android/blob/a1386beed8d8215fe8ab79c501c1ec237ae8a785/app/src/main/kotlin/org/wordpress/aztec/demo/MainActivity.kt#L443
    the More Button will disappear from the toolbar, but this is not the case if we remove your `aztec.addPlugin(CssBackgroundColorPlugin())` because it is in the main layout sections of simple and advanced toolbars instead of the plugin section.)

Thanks again for the changes here.

@cameronvoell can you verify if the 1st and 3th items are ok? I believe i fixed the #914 also i removed the button from the toolbar and changed to be a plugin, still trying to figure out the second item(highlighting problem)

arctouch-felipevaladares · 2020-07-03T19:05:17Z

@cameronvoell highlight problem fixed 👍

…groundColorSpan

…acilitate the use on recyclerview with multiple edittext for example

cameronvoell · 2020-07-31T02:20:10Z

aztec/src/main/kotlin/org/wordpress/aztec/formatting/InlineFormatter.kt

@@ -238,13 +262,6 @@ class InlineFormatter(editor: AztecText, val codeStyle: CodeStyle) : AztecFormat
        joinStyleSpans(start, end)
    }

-    fun removeInlineCssStyle(start: Int = selectionStart, end: Int = selectionEnd) {


I am a little suspicious that removing this code might have side effects, though I'm not sure what they are yet. Also this does not seem to fix all cases of #914. For example, if you highlight the first red italic text in the sample text and turn off italic, and then open html you will see the color attribute is gone, and returning from html the color will be gone in the display text.

I did see that background color no longer disappears when you remove a text format so that is an improvement. Perhaps we leave this code as it was since it does not affect background, and it does not seem to completely fix #914 either.

This code is weird, it just remove everything without any rules, not sure if it will break something, maybe it's a work around to something? it really doesn't make any sense to me

cameronvoell · 2020-07-31T02:20:52Z

aztec/src/main/kotlin/org/wordpress/aztec/formatting/InlineFormatter.kt

@@ -221,8 +247,6 @@ class InlineFormatter(editor: AztecText, val codeStyle: CodeStyle) : AztecFormat
                editableText.removeSpan(it)
            }
        }
-        // remove the CSS style span
-        removeInlineCssStyle()


Suggest we keep this for now since it does not affect background color, see previous https://github.com/wordpress-mobile/AztecEditor-Android/pull/911/files#r463370943

ok, if really doesn't affect bg color i think we can keep, but its something to take a look in the future, that method doesn't make much sense

cameronvoell · 2020-07-31T02:38:12Z

aztec/src/main/kotlin/org/wordpress/aztec/spans/AztecBackgroundColorSpan.kt

+
+class AztecBackgroundColorSpan(
+        val color: Int,
+        val alpha: Int = 220,


I think only color is ever used as a constructor parameter from here. The other parameters can probably be moved to be class properties. What do you think?

no problem, i'll take a look into this

cameronvoell · 2020-07-31T02:54:21Z

Hello @arctouch-felipevaladares , thanks for the updates, and sorry for the delay with the review.

@cameronvoell highlight problem fixed 👍

Nice fix here.

@cameronvoell can you verify if the 1st and 3th items are ok? I believe i fixed the #914 also i removed the button from the toolbar and changed to be a plugin

I left some comments about the 1st item, along with a few other comments on the code. The 3rd item for making the background button functionality an optional plugin is looking good as well 👍

I did find one other issue that is related to the highlight state of the Background button. It looks like after highlighting some text and then navigating elsewhere the button stays highlighted. The button should function like the other buttons in that it is highlighted only when placed above or highlighting text that has that property, and then it should automatically go to inactive state when moved elsewhere (see bold or italic button for example).

maxme · 2020-08-03T08:31:24Z

Just a note that we're changing the project license, since this haven't landed yet it's not a problem, but if the patch land after the switch, it will be done under the MPL v2.

Ref. #922

yuvalgnessin-qz · 2020-08-11T19:43:25Z

Hi @felipevaladares :) do you think you might be able to respond to @cameronvoell's comments in the near term? If not, I might be interested in taking over this effort. Let me know!

felipevaladares · 2020-08-11T20:49:16Z

Hi @felipevaladares :) do you think you might be able to respond to @cameronvoell's comments in the near term? If not, I might be interested in taking over this effort. Let me know!

Hi @yuvalgnessin-qz i will do that this week :)

felipevaladares · 2020-08-11T20:55:52Z

Just a note that we're changing the project license, since this haven't landed yet it's not a problem, but if the patch land after the switch, it will be done under the MPL v2.

Ref. #922

No problem, i agree

starshipcoder · 2020-10-09T11:27:00Z

I suggest the background icon take the current background color, so moving cursor could modify this color too

And it will be ready for a color picker

yuvalgnessin-qz · 2021-01-23T01:19:55Z

Hello @cameronvoell and others! Since @felipevaladares was not available to address the changes you requested, I have opened a new PR #934 implemented by @lvcasasanta to replace this one. It addresses all the changes you have requested.

Please take a look as soon as you can. Thank you!

SiobhyB · 2023-04-06T08:51:47Z

I'm going ahead to close this PR, as it's been superseded by #1041.

felipevaladares added 6 commits May 26, 2020 02:10

New Feature, support for BackgroundColorSpan:

f00c599

- Added CssBackgroundColorPlugin based on CssUnderlinePlugin - Added AztecBackgroundColorSpan - General changes to implement and use BackgroundColorSpan

- Added new icons for ToolbarAction.BACKGROUND

8dd7d9a

- Example of multiple bg colors

34175a8

- General improvements, removing code that is not necessary

6ae030b

- Fix bug when handling with multiple bg colors

cc8533d

- Fix ktlint issue

ded1c56

felipevaladares mentioned this pull request May 31, 2020

BackgroundColorSpan support? #910

Closed

felipevaladares marked this pull request as draft May 31, 2020 22:45

felipevaladares marked this pull request as ready for review May 31, 2020 22:46

cameronvoell self-requested a review June 8, 2020 18:15

cameronvoell suggested changes Jun 12, 2020

View reviewed changes

arctouch-felipevaladares mentioned this pull request Jun 30, 2020

Format text then remove format removes color style #914

Open

felipevaladares added 7 commits June 30, 2020 15:24

- Fix bug wordpress-mobile#914

536d95d

- Improved bg color implementation, fix bugs when using multiple back…

c6a1bcf

…ground colors

- Fix lint errors

3213bc2

-

a96ef7c

- Fix broken test, changing to 'em' because it comes first on the ita…

ba6962d

…lic action

- Removing bg color button from toolbar, Added BackgroundColorButton …

a95d50e

…as plugin so that could be optional

- Removing bg color from ToolbarAction

936884e

- Fix highlight color not appearing when BackgroundColorSpan is applied

40503ef

felipevaladares added 3 commits July 3, 2020 16:07

- Fix lint error, added alpha for custom implementations on AztecBack…

c876547

…groundColorSpan

- Added possibility to clear references from toolbar and editor, to f…

3a99a6e

…acilitate the use on recyclerview with multiple edittext for example

- Fix lint error

a00fd4a

arctouch-felipevaladares mentioned this pull request Jul 22, 2020

PR Review #920

Closed

cameronvoell reviewed Jul 31, 2020

View reviewed changes

yuvalgnessin-qz mentioned this pull request Jan 23, 2021

Added backgroundColorSpan support v2 #934

Closed

yuvalgnessin-qz mentioned this pull request Apr 5, 2023

Added backgroundColorSpan support v3 #1041

Merged

SiobhyB closed this Apr 6, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added backgroundColorSpan support #911

Added backgroundColorSpan support #911

felipevaladares commented May 26, 2020 •

edited

Loading

duyvt88 commented May 31, 2020

felipevaladares commented May 31, 2020

felipevaladares commented May 31, 2020

duyvt88 commented Jun 1, 2020

felipevaladares commented Jun 1, 2020

duyvt88 commented Jun 6, 2020

felipevaladares commented Jun 8, 2020

felipevaladares commented Jun 8, 2020

cameronvoell left a comment •

edited

Loading

felipevaladares commented Jun 12, 2020

duyvt88 commented Jun 18, 2020

arctouch-felipevaladares commented Jul 3, 2020

arctouch-felipevaladares commented Jul 3, 2020

cameronvoell Jul 31, 2020

felipevaladares Aug 11, 2020

cameronvoell Jul 31, 2020 •

edited

Loading

felipevaladares Aug 11, 2020

cameronvoell Jul 31, 2020

felipevaladares Aug 11, 2020

cameronvoell commented Jul 31, 2020 •

edited

Loading

maxme commented Aug 3, 2020

yuvalgnessin-qz commented Aug 11, 2020

felipevaladares commented Aug 11, 2020

felipevaladares commented Aug 11, 2020

starshipcoder commented Oct 9, 2020

yuvalgnessin-qz commented Jan 23, 2021

SiobhyB commented Apr 6, 2023

Added backgroundColorSpan support #911

Added backgroundColorSpan support #911

Conversation

felipevaladares commented May 26, 2020 • edited Loading

Test

Review

duyvt88 commented May 31, 2020

felipevaladares commented May 31, 2020

felipevaladares commented May 31, 2020

duyvt88 commented Jun 1, 2020

felipevaladares commented Jun 1, 2020

duyvt88 commented Jun 6, 2020

felipevaladares commented Jun 8, 2020

felipevaladares commented Jun 8, 2020

cameronvoell left a comment • edited Loading

Choose a reason for hiding this comment

felipevaladares commented Jun 12, 2020

duyvt88 commented Jun 18, 2020

arctouch-felipevaladares commented Jul 3, 2020

arctouch-felipevaladares commented Jul 3, 2020

cameronvoell Jul 31, 2020

Choose a reason for hiding this comment

felipevaladares Aug 11, 2020

Choose a reason for hiding this comment

cameronvoell Jul 31, 2020 • edited Loading

Choose a reason for hiding this comment

felipevaladares Aug 11, 2020

Choose a reason for hiding this comment

cameronvoell Jul 31, 2020

Choose a reason for hiding this comment

felipevaladares Aug 11, 2020

Choose a reason for hiding this comment

cameronvoell commented Jul 31, 2020 • edited Loading

maxme commented Aug 3, 2020

yuvalgnessin-qz commented Aug 11, 2020

felipevaladares commented Aug 11, 2020

felipevaladares commented Aug 11, 2020

starshipcoder commented Oct 9, 2020

yuvalgnessin-qz commented Jan 23, 2021

SiobhyB commented Apr 6, 2023

felipevaladares commented May 26, 2020 •

edited

Loading

cameronvoell left a comment •

edited

Loading

cameronvoell Jul 31, 2020 •

edited

Loading

cameronvoell commented Jul 31, 2020 •

edited

Loading