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.

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

[WIP] Fix issue where shadows may not appear on views #1227

Closed
wants to merge 1 commit into from

Conversation

lyahdav
Copy link
Collaborator

@lyahdav lyahdav commented Jul 5, 2022

Credit goes to @appden. This isn't ready to commit because NSShadow.shadowBlurRadius is slightly different than CALayer.shadowRadius.

Fixes #824

Credit goes to @appden. This isn't ready to commit because NSShadow.shadowBlurRadius is slightly different than CALayer.shadowRadius.
@pull-bot
Copy link

pull-bot commented Jul 5, 2022

Messages
📖

📋 Missing Changelog - Can you add a Changelog? To do so, add a "## Changelog" section to your PR description. A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

CATEGORY may be:
  • General
  • macOS
  • iOS
  • Android
  • JavaScript
  • Internal (for changes that do not need to be called out in the release notes)

TYPE may be:

  • Added, for new features.
  • Changed, for changes in existing functionality.
  • Deprecated, for soon-to-be removed features.
  • Removed, for now removed features.
  • Fixed, for any bug fixes.
  • Security, in case of vulnerabilities.

MESSAGE may answer "what and why" on a feature level. Use this to briefly tell React Native users about notable changes.

📖 📋 Missing Test Plan - Can you add a Test Plan? To do so, add a "## Test Plan" section to your PR description. A Test Plan lets us know how these changes were tested.
📖 📋 Missing Summary - Can you add a Summary? To do so, add a "## Summary" section to your PR description. This is a good place to explain the motivation for making this change.

Generated by 🚫 dangerJS against 31c8034

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 71e8921
Branch: main

@Saadnajmi
Copy link
Collaborator

Thanks! I didn't realize there was a shadowBlurRadius separate from shadowRadius :)

@mischreiber @lyahdav

@Saadnajmi
Copy link
Collaborator

@lyahdav a quick update from our side. We have a bug on our plate to to fix shadows on macOS, but the engineer that's assigned to is on vacation, so we'll revisit once they are back. When I locally tested this, it almost felt like they were two shadows showing up, one from the original iOS layer.shadow* setters, and one from the NSShadow property you added. As for NSShadow.shadowBlurRadius vs shadowRadius, Apple documentation has me thinking they both control the same thing :)

@Saadnajmi
Copy link
Collaborator

From local testing, it seems the shadow on the layer through the props in RCTViewManager is still showing up half the time, so it feels like we have 2 shadows:

Could you instead change the RCTVIewManager macros to CUSTOM_VIEW_PROPERTY like we do here so we never set it directly on the layer? #1035

@lyahdav
Copy link
Collaborator Author

lyahdav commented Aug 9, 2022

@Saadnajmi I wasn't planning on trying to land this anytime soon, I just made this PR to share WIP. You can make a new PR with your suggested change.

@ghost ghost removed the Needs: Author Feedback label Aug 9, 2022
@lyzhan7 lyzhan7 mentioned this pull request Aug 10, 2022
4 tasks
@christophpurrer
Copy link

It seems that issue was fixed in #1352

@Saadnajmi Saadnajmi closed this Aug 11, 2022
@Saadnajmi
Copy link
Collaborator

It seems that issue was fixed in #1352

Yep! We needed a little bit of extra code, so do give it a look.

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.

Component shadows do not always applies in 0.63.37
6 participants