Skip to content

Set link text color and link underline from JS#1101

Closed
marecar3 wants to merge 1 commit intoissue/697_change_color_of_text_and_linksfrom
issue/697_change_color_of_text_and_links_android
Closed

Set link text color and link underline from JS#1101
marecar3 wants to merge 1 commit intoissue/697_change_color_of_text_and_linksfrom
issue/697_change_color_of_text_and_links_android

Conversation

@marecar3
Copy link
Copy Markdown
Contributor

@marecar3 marecar3 commented Jun 7, 2019

It is Android update for PR : #1086

With this PR we are allowing JS side to change the link text color on Android, which will also turn on the underline option on the link.

In order to achieve this, I have created a branch on AztecEditorAndroid repo with change which allows us to set link style: https://github.com/wordpress-mobile/AztecEditor-Android/compare/fix/make_link_style_public

Currently, there is a problem with how props are propagated to native code. With this line of code https://github.com/WordPress/gutenberg/pull/16016/files#diff-4828a21853e899e5a36faecfa96d83e8R817 , Android native code will receive linkTextColor prop after text prop, which means it's too late to change link style as text is already drawn on the screen.

The only solution that came on my mind was to add linkTextColor as property in text prop: https://github.com/WordPress/gutenberg/pull/16050/files .

@SergioEstevao suggested that we try to set text again when we receive linkTextColor prop, which I tried and it's working, but I think it's not the best option for Android regarding performance, as we will force to setText on components that don't have a link (as linkTextColor prop is always passed from JS side) and if we want to set a text only on components that has a link, we would need to check if each component has it which also seems an impact on performance.

Let's open discussion here @daniloercoli @hypest , maybe we can figure out a better solution than this.

@SergioEstevao
Copy link
Copy Markdown
Contributor

SergioEstevao commented Jun 7, 2019

The only solution that came on my mind was to add linkTextColor as property in text prop:

I don't know the details of Aztec implementation in Android, but do we need to do this for links but not for the regular text color?

@daniloercoli
Copy link
Copy Markdown
Contributor

daniloercoli commented Jun 10, 2019

Currently, there is a problem with how props are propagated to native code. With this line of code https://github.com/WordPress/gutenberg/pull/16016/files#diff-4828a21853e899e5a36faecfa96d83e8R817 , Android native code will receive linkTextColor prop after text prop, which means it's too late to change link style as text is already drawn on the screen.

Try to move linkTextColor before text prop, right below Style, it should work.
Not quite sure this can be considered as a valid solution, but worth to try.

Otherwise we may try redrawing the Aztec editor when linkTextColor is set on the wrapper.

@hypest
Copy link
Copy Markdown
Contributor

hypest commented Jun 10, 2019

I'll try @daniloercoli 's idea and try to move this PR forward, in an attempt to make it to today's code freeze.

@hypest
Copy link
Copy Markdown
Contributor

hypest commented Jun 14, 2019

For the release, we opted for a hardcoded setting of the link color #1109

@marecar3
Copy link
Copy Markdown
Contributor Author

For the release, we opted for a hardcoded setting of the link color #1109

In that case, probably we can close this one.

@marecar3 marecar3 closed this Jun 18, 2019
@SergioEstevao SergioEstevao deleted the issue/697_change_color_of_text_and_links_android branch November 8, 2019 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants