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.

Sign up for GitHub

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

Jump to bottom

BREAKING - default underlineColorAndroid to transparent #18988

Closed
wants to merge 1 commit into from
Closed

BREAKING - default underlineColorAndroid to transparent #18988

wants to merge 1 commit into from

Conversation

mikaoelitiana
Copy link
Contributor

@mikaoelitiana mikaoelitiana commented Apr 23, 2018 •
edited by janicduplessis
Loading

Set default underlineColorAndroid to transparent.

Fixes #18938

Test Plan

Use a TextInput in a component without defining underlineColorAndroid, the underline color should be transparent.

Related PRs

Release Notes

[ANDROID] [BREAKING] [TextInput] - set default underlineColorAndroid to transparent

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Apr 23, 2018
@janicduplessis janicduplessis changed the title feat: default underlineColorAndroid to transparent BREAKING - default underlineColorAndroid to transparent Apr 24, 2018
@janicduplessis
Copy link
Contributor

janicduplessis commented Apr 24, 2018 •
edited
Loading

@mikaoelitiana Thanks for the PR! I just updated the title and release not to mention this is a breaking change.

@mjesun Could you have a quick look to see if this could break anything @fb before we land. TextInputs that don't define underlineColorAndroid are the ones that will be affected.

@mjesun
Copy link
Contributor

mjesun commented Apr 24, 2018

@janicduplessis I can import the PR internally, but I'm probably not the best one to check this 🙂

cc'ing @yungsters who knows better than I do 😀

@janicduplessis
Copy link
Contributor

@mjesun Thanks for redirecting my request to the right person, I wasn’t sure who to ping on this one 😁

@yungsters
Copy link
Contributor

@mdvacca dug into this a bit just now and we were both surprised that the default was not transparent after looking here at ReactTextInputManager.java.

It turns out that at least in API 19, the default was a black underline. In API 25, the default is now no underline. So making transparent the default makes a lot of sense.

This change makes a lot of sense (instead of setting it in the view manager) because in the event that underlineColorAndroid is provided, it's likely better to send this value over the bridge and update the underlying view one time rather than initializing it to transparent and then re-setting it.

I'll get this merged internally. Thanks for the pull request!

facebook-github-bot
facebook-github-bot reviewed Apr 25, 2018
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@yungsters has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@janicduplessis
Copy link
Contributor

@yungsters @mdvacca Thanks for looking at this, yea from what I understand how it works is that it applies a color filter to the underline drawable, which is by default based on the theme color. In this case clearing the color filter will just bring back the color to the default and not remove the underline. Applying a transparent color filter using default props seemed like the best way to make it work.

macdoum1 pushed a commit to macdoum1/react-native that referenced this pull request Jun 28, 2018
@mikaoelitiana @macdoum1
BREAKING - default underlineColorAndroid to transparent
eefc855 
Summary:
Set default `underlineColorAndroid` to `transparent`.
<!--
  Required: Write your motivation here.
  If this PR fixes an issue, type "Fixes #issueNumber" to automatically close the issue when the PR is merged.
-->

Fixes facebook#18938

<!--
  Required: Write your test plan here. If you changed any code, please provide us with
  clear instructions on how you verified your changes work. Bonus points for screenshots and videos!
-->

Use a TextInput in a component without defining `underlineColorAndroid`, the underline color should be transparent.

<!--
  Does this PR require a documentation change?
  Create a PR at https://github.com/facebook/react-native-website and add a link to it here.
-->

<!--
  Required.
  Help reviewers and the release process by writing your own release notes. See below for an example.
-->

[ANDROID] [BREAKING] [TextInput] - set default underlineColorAndroid to transparent

<!--
  **INTERNAL and MINOR tagged notes will not be included in the next version's final release notes.**

    CATEGORY
  [----------]      TYPE
  [ CLI      ] [-------------]    LOCATION
  [ DOCS     ] [ BREAKING    ] [-------------]
  [ GENERAL  ] [ BUGFIX      ] [ {Component} ]
  [ INTERNAL ] [ ENHANCEMENT ] [ {Filename}  ]
  [ IOS      ] [ FEATURE     ] [ {Directory} ]   |-----------|
  [ ANDROID  ] [ MINOR       ] [ {Framework} ] - | {Message} |
  [----------] [-------------] [-------------]   |-----------|

 EXAMPLES:

 [IOS] [BREAKING] [FlatList] - Change a thing that breaks other things
 [ANDROID] [BUGFIX] [TextInput] - Did a thing to TextInput
 [CLI] [FEATURE] [local-cli/info/info.js] - CLI easier to do things with
 [DOCS] [BUGFIX] [GettingStarted.md] - Accidentally a thing/word
 [GENERAL] [ENHANCEMENT] [Yoga] - Added new yoga thing/position
 [INTERNAL] [FEATURE] [./scripts] - Added thing to script that nobody will see
-->
Closes facebook#18988

Reviewed By: mdvacca

Differential Revision: D7765569

Pulled By: yungsters

fbshipit-source-id: f7ad57a46fc0d18b47271ca39faae8c635995fbb
@martnst
Copy link

martnst commented Feb 12, 2019

@mikaoelitiana While I can see the point of changing the default underlineColorAndroid to transparent - I think this change was a little bit poorly executed.

image

*Generally speaking I think changing a default value should effect the ability to set some value to achieve the behaviour before. *

@martnst martnst mentioned this pull request Feb 12, 2019
Closed
fabOnReact added a commit to fabOnReact/react-native that referenced this pull request Feb 5, 2021
@fabOnReact
fix - underlineColorAndroid not working on API 21
7c5587e 
on API 21 we change the logic as follows to fix underlineColorAndroid:
- we store the bottomBorderColor in a local variable
- we set the bottomBorderColor to the same color of
  underlineColorAndroid
- we set the underlineColorAndroid
- we re-set the bottomBorderColor to the previous color

As by Facebook design it defaults to
transparent

facebook#18938
facebook#18988

but it is important to notice how this default is now not anymore used
as JAVA sets a new default of transparent for all colors (null is not
passed to this setter)

facebook#29412 (comment)
@fabOnReact fabOnReact mentioned this pull request Feb 5, 2021
Closed
facebook-github-bot pushed a commit that referenced this pull request Mar 10, 2022
@fabOnReact @facebook-github-bot
underlineColor transparent not working on API 21 (#30897)
52aee50 
Summary:
This issue fixes #29379 `underlineColorAndroid'='transparent'` does not work on Android API 21.
Setting `style: { bottomBorderColor: 'transparent'}` fixes the issue.

The following steps fix underlineColorAndroid on Android API 21:
- Store the bottomBorderColor in a local variable
- Then set the bottomBorderColor to the same color of underlineColorAndroid
- Set underlineColorAndroid
- Then Set the bottomBorderColor to the previous color previously stored in the local variable

This change requires `ReactViewBackgroundDrawable` method `getBorderColor` to be public and accessible from `ReactTextInputManager` to retrieve the border color.

https://github.com/facebook/react-native/blob/6061b7928320c64a94716ce3d6646667ebf3f6b5/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java#L1231-L1236

Related Discussions #18938 #18988 #29412 (comment)

More Information at fabOnReact/react-native-notes#4 (comment)

## Changelog

[Android] [Fixed] - Fix underlineColorAndroid transparent not working on API 21

Pull Request resolved: #30897

Test Plan:
This changes fix the Java API which can not be tested as explained in commit 709a441
The java TextInputTest was excluded from the test suite in commit 709a441 as they need the Yoga libraries to run

**<details><summary>CLICK TO OPEN TESTS RESULTS - API 21</summary>**
<p>

Does not show underline by default (`transparent`)

```javascript
<TextInput />
```

<image src="https://user-images.githubusercontent.com/24992535/107060953-dee34d00-67d7-11eb-8f01-360dbb1420b8.png" width="150" />

```javascript
<TextInput underlineColorAndroid="red" />
```

<image src="https://user-images.githubusercontent.com/24992535/107061134-194cea00-67d8-11eb-9da1-9780b1aa8294.png" width="150" />

```javascript
<TextInput
  underlineColorAndroid="green"
  style={
     {
        borderBottomColor: 'red',
        borderBottomWidth: 2,
        borderTopColor: 'black',
        borderTopWidth: 4
     }
   }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107062411-9167df80-67d9-11eb-810c-749992d8d2d3.png" width="150" />

```javascript
<TextInput
  style={
     {
        borderBottomColor: 'red',
        borderBottomWidth: 2,
        borderTopColor: 'black',
        borderTopWidth: 4
     }
   }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107062735-e6a3f100-67d9-11eb-93c3-02cd768f8421.png" width="150" />

```javascript
<TextInput
  underlineColorAndroid="blue"
  style={
    {
      borderBottomColor: 'red',
      borderBottomWidth: 2,
      borderTopColor: 'black',
      borderTopWidth: 4,
      borderLeftColor: 'pink',
      borderLeftWidth: 7,
      borderRightColor: 'yellow',
      borderRightWidth: 7,
    }
  }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107063263-75b10900-67da-11eb-97ab-316736d525a2.png" width="150" />

```javascript
<TextInput
  underlineColorAndroid="transparent"
  style={
    {
      borderBottomColor: 'red',
      borderBottomWidth: 2,
      borderTopColor: 'black',
      borderTopWidth: 4,
      borderLeftColor: 'pink',
      borderLeftWidth: 7,
      borderRightColor: 'yellow',
      borderRightWidth: 7,
    }
  }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107063332-8792ac00-67da-11eb-82fc-99793bf4d49d.png" width="150" />

</p>
</details>

Reviewed By: cortinico

Differential Revision: D33906415

Pulled By: lunaleaps

fbshipit-source-id: bd7efe4aac40ad701aec83f051ac40dcd4a99cda
alphatrl pushed a commit to taskade/react-native that referenced this pull request Dec 6, 2022
@fabOnReact @alphatrl
underlineColor transparent not working on API 21 (facebook#30897)
ff9084e 
Summary:
This issue fixes facebook#29379 `underlineColorAndroid'='transparent'` does not work on Android API 21.
Setting `style: { bottomBorderColor: 'transparent'}` fixes the issue.

The following steps fix underlineColorAndroid on Android API 21:
- Store the bottomBorderColor in a local variable
- Then set the bottomBorderColor to the same color of underlineColorAndroid
- Set underlineColorAndroid
- Then Set the bottomBorderColor to the previous color previously stored in the local variable

This change requires `ReactViewBackgroundDrawable` method `getBorderColor` to be public and accessible from `ReactTextInputManager` to retrieve the border color.

https://github.com/facebook/react-native/blob/6061b7928320c64a94716ce3d6646667ebf3f6b5/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java#L1231-L1236

Related Discussions facebook#18938 facebook#18988 facebook#29412 (comment)

More Information at fabOnReact/react-native-notes#4 (comment)

## Changelog

[Android] [Fixed] - Fix underlineColorAndroid transparent not working on API 21

Pull Request resolved: facebook#30897

Test Plan:
This changes fix the Java API which can not be tested as explained in commit facebook@709a441
The java TextInputTest was excluded from the test suite in commit facebook@709a441 as they need the Yoga libraries to run

**<details><summary>CLICK TO OPEN TESTS RESULTS - API 21</summary>**
<p>

Does not show underline by default (`transparent`)

```javascript
<TextInput />
```

<image src="https://user-images.githubusercontent.com/24992535/107060953-dee34d00-67d7-11eb-8f01-360dbb1420b8.png" width="150" />

```javascript
<TextInput underlineColorAndroid="red" />
```

<image src="https://user-images.githubusercontent.com/24992535/107061134-194cea00-67d8-11eb-9da1-9780b1aa8294.png" width="150" />

```javascript
<TextInput
  underlineColorAndroid="green"
  style={
     {
        borderBottomColor: 'red',
        borderBottomWidth: 2,
        borderTopColor: 'black',
        borderTopWidth: 4
     }
   }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107062411-9167df80-67d9-11eb-810c-749992d8d2d3.png" width="150" />

```javascript
<TextInput
  style={
     {
        borderBottomColor: 'red',
        borderBottomWidth: 2,
        borderTopColor: 'black',
        borderTopWidth: 4
     }
   }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107062735-e6a3f100-67d9-11eb-93c3-02cd768f8421.png" width="150" />

```javascript
<TextInput
  underlineColorAndroid="blue"
  style={
    {
      borderBottomColor: 'red',
      borderBottomWidth: 2,
      borderTopColor: 'black',
      borderTopWidth: 4,
      borderLeftColor: 'pink',
      borderLeftWidth: 7,
      borderRightColor: 'yellow',
      borderRightWidth: 7,
    }
  }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107063263-75b10900-67da-11eb-97ab-316736d525a2.png" width="150" />

```javascript
<TextInput
  underlineColorAndroid="transparent"
  style={
    {
      borderBottomColor: 'red',
      borderBottomWidth: 2,
      borderTopColor: 'black',
      borderTopWidth: 4,
      borderLeftColor: 'pink',
      borderLeftWidth: 7,
      borderRightColor: 'yellow',
      borderRightWidth: 7,
    }
  }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107063332-8792ac00-67da-11eb-82fc-99793bf4d49d.png" width="150" />

</p>
</details>

Reviewed By: cortinico

Differential Revision: D33906415

Pulled By: lunaleaps

fbshipit-source-id: bd7efe4aac40ad701aec83f051ac40dcd4a99cda
Saadnajmi pushed a commit to Saadnajmi/react-native-macos that referenced this pull request Jan 15, 2023
@fabOnReact @Saadnajmi
underlineColor transparent not working on API 21 (facebook#30897)
d087821 
Summary:
This issue fixes facebook#29379 `underlineColorAndroid'='transparent'` does not work on Android API 21.
Setting `style: { bottomBorderColor: 'transparent'}` fixes the issue.

The following steps fix underlineColorAndroid on Android API 21:
- Store the bottomBorderColor in a local variable
- Then set the bottomBorderColor to the same color of underlineColorAndroid
- Set underlineColorAndroid
- Then Set the bottomBorderColor to the previous color previously stored in the local variable

This change requires `ReactViewBackgroundDrawable` method `getBorderColor` to be public and accessible from `ReactTextInputManager` to retrieve the border color.

https://github.com/facebook/react-native/blob/6061b7928320c64a94716ce3d6646667ebf3f6b5/ReactAndroid/src/main/java/com/facebook/react/views/view/ReactViewBackgroundDrawable.java#L1231-L1236

Related Discussions facebook#18938 facebook#18988 facebook#29412 (comment)

More Information at fabOnReact/react-native-notes#4 (comment)

## Changelog

[Android] [Fixed] - Fix underlineColorAndroid transparent not working on API 21

Pull Request resolved: facebook#30897

Test Plan:
This changes fix the Java API which can not be tested as explained in commit facebook@709a441
The java TextInputTest was excluded from the test suite in commit facebook@709a441 as they need the Yoga libraries to run

**<details><summary>CLICK TO OPEN TESTS RESULTS - API 21</summary>**
<p>

Does not show underline by default (`transparent`)

```javascript
<TextInput />
```

<image src="https://user-images.githubusercontent.com/24992535/107060953-dee34d00-67d7-11eb-8f01-360dbb1420b8.png" width="150" />

```javascript
<TextInput underlineColorAndroid="red" />
```

<image src="https://user-images.githubusercontent.com/24992535/107061134-194cea00-67d8-11eb-9da1-9780b1aa8294.png" width="150" />

```javascript
<TextInput
  underlineColorAndroid="green"
  style={
     {
        borderBottomColor: 'red',
        borderBottomWidth: 2,
        borderTopColor: 'black',
        borderTopWidth: 4
     }
   }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107062411-9167df80-67d9-11eb-810c-749992d8d2d3.png" width="150" />

```javascript
<TextInput
  style={
     {
        borderBottomColor: 'red',
        borderBottomWidth: 2,
        borderTopColor: 'black',
        borderTopWidth: 4
     }
   }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107062735-e6a3f100-67d9-11eb-93c3-02cd768f8421.png" width="150" />

```javascript
<TextInput
  underlineColorAndroid="blue"
  style={
    {
      borderBottomColor: 'red',
      borderBottomWidth: 2,
      borderTopColor: 'black',
      borderTopWidth: 4,
      borderLeftColor: 'pink',
      borderLeftWidth: 7,
      borderRightColor: 'yellow',
      borderRightWidth: 7,
    }
  }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107063263-75b10900-67da-11eb-97ab-316736d525a2.png" width="150" />

```javascript
<TextInput
  underlineColorAndroid="transparent"
  style={
    {
      borderBottomColor: 'red',
      borderBottomWidth: 2,
      borderTopColor: 'black',
      borderTopWidth: 4,
      borderLeftColor: 'pink',
      borderLeftWidth: 7,
      borderRightColor: 'yellow',
      borderRightWidth: 7,
    }
  }
/>
```

<image src="https://user-images.githubusercontent.com/24992535/107063332-8792ac00-67da-11eb-82fc-99793bf4d49d.png" width="150" />

</p>
</details>

Reviewed By: cortinico

Differential Revision: D33906415

Pulled By: lunaleaps

fbshipit-source-id: bd7efe4aac40ad701aec83f051ac40dcd4a99cda
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@janicduplessis janicduplessis janicduplessis approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Platform: Android Android applications.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Make the default for underlineColorAndroid transparent
7 participants
@mikaoelitiana @janicduplessis @mjesun @yungsters @martnst @facebook-github-bot @react-native-bot