Add shadow inconsistency opacity fix with updated tests #1402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Saadnajmi
reviewed
Aug 30, 2022
packages/rn-tester/js/examples/AnimatedGratuitousApp/AnExSet.js
Outdated
Show resolved
Hide resolved
Saadnajmi
approved these changes
Aug 30, 2022
chiuam
approved these changes
Aug 31, 2022
mischreiber
approved these changes
Sep 1, 2022
|
Turns out textShadowOpacity does not exist (even though shadowOpacity does) - wasn't sure if this was necessary but went ahead and made sure the opacity was set to 1 in the textShadowColor. Also don't really know why it wasn't complaining before about omitting width... |
…-macos into lyzhan-shadowfix2.1
Saadnajmi
reviewed
Sep 1, 2022
packages/rn-tester/js/examples/AnimatedGratuitousApp/AnExSet.js
Outdated
Show resolved
Hide resolved
lyzhan7
added a commit
to lyzhan7/react-native-macos
that referenced
this pull request
Sep 1, 2022
* Redo shadow opacity consistency fix * Update test that was introducing redboxes * Fix yarn flow-check-ios errors * Add comment linking to github issue * Update comment
lyzhan7
added a commit
to lyzhan7/react-native-macos
that referenced
this pull request
Sep 1, 2022
* Redo shadow opacity consistency fix * Update test that was introducing redboxes * Fix yarn flow-check-ios errors * Add comment linking to github issue * Update comment
10 tasks
ghost
pushed a commit
to microsoft/fluentui-react-native
that referenced
this pull request
Sep 9, 2022
### Platforms Impacted - [x] iOS - [x] macOS - [ ] win32 (Office) - [ ] windows - [ ] android ### Description of changes This is a follow up to investigation that happened here: microsoft/react-native-macos#1402 After fixing a bug related to shadow consistency in react-native-macOS, one of the tests in react-native-macOS started failing. The issue seemed to be with the test itself (and its still unclear why the test only started failing after the bug fix) - the test was setting shadow props (ex. shadowColor, shadowOpacity, shadowOffset, shadowRadius) on a Text element, when [Text actually has its own specific shadow props](https://reactnative.dev/docs/text-style-props#textshadowcolor) - i.e. textShadowColor, textShadowOffset, textShadowRadius (interestingly textShadowOpacity is not a thing) I considered updating the Shadow component to also work for Text, but decided against it because Shadows on Text doesn't seem to be an actual use case - usually it's a Shadow on a View that has text on it. This PR - updates the jest tests that were doing the same thing as the test in react-native-macOS, the Text should be wrapped in a view and then the Shadow is wrapped around that - updates the documentation to reflect this change ### Verification I tried the following code that was in the Shadow jest tests in FluentTester: ```javascript <Shadow shadowToken={theme.shadows.shadow64}> <Text> test </Text> </Shadow> ``` and this actually caused a red box to appear (same error as in the react-native-macOS test that started failing) <img width="335" alt="Screen Shot 2022-09-07 at 12 02 33 PM" src="https://user-images.githubusercontent.com/78454019/188957094-140956b4-80c0-479e-95eb-814f19d29277.png"> Changing the code to this stopped the red box from showing up. ```javascript <Shadow shadowToken={theme.shadows.shadow64}> <View> <Text> test </Text> </View> </Shadow> ``` I would have expected the jest tests to fail on any code that causes a red box, if anyone has any insight on why the jest tests did not catch this please let me know (best guess is because the red box error is coming from native code, and jest tests don't catch those). No visible changes in the tester. ### Pull request checklist This PR has considered (when applicable): - [x] Automated Tests - [x] Documentation and examples - [ ] Keyboard Accessibility - [ ] Voiceover - [ ] Internationalization and Right-to-left Layouts
Saadnajmi
pushed a commit
to Saadnajmi/react-native-macos
that referenced
this pull request
Nov 16, 2022
* Redo shadow opacity consistency fix * Update test that was introducing redboxes * Fix yarn flow-check-ios errors * Add comment linking to github issue * Update comment
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please select one of the following
Planning to make the same change upstream into RN core
Summary
Revisiting a598ee4 - this got reverted in #1385 because it introduced an issue with a test. After more investigation, it seems like the test was incorrect and the error was expected. This PR is to add back the original changes + fix an ifdef bug that was in the original change + update the test.
The issue with the test is that the react native Text component has its own props for shadows - i.e. textShadowColor, textShadowRadius, etc and the shadow props that View uses does not work on Text (i.e. shadowColor, shadowRadius, etc.). Not entirely sure how this test was passing without any errors before my change, it seems like something about the change is causing (expected) errors to actually show up.
Changelog
[macOS] [Fixed] - Shadow consistency opacity fix with test updates
Test Plan
Confirmed that on iOS and macOS, setting shadowColor/shadowOffset/etc. does not work on Text, only textShadowColor/textShadowOffset/etc works. Confirmed that after updating the tests, the tests pass.