Skip to content

Add boxShadow support for modern React Native styling#30

Open
fireinnti wants to merge 1 commit intozeplin:mainfrom
fireinnti:main
Open

Add boxShadow support for modern React Native styling#30
fireinnti wants to merge 1 commit intozeplin:mainfrom
fireinnti:main

Conversation

@fireinnti
Copy link
Copy Markdown

Change description

Adds box shadow property to react native extension.

Type of change

  • New feature (adds functionality)

Related issues

Fix #1

Checklists

Development

  • Lint rules pass locally
  • Application changes have been tested thoroughly
  • Automated tests covering modified code pass

Security

  • Security impact of change has been considered
  • Code follows company security practices and guidelines

Code review

  • Pull request has a descriptive title and context useful to a reviewer. Screenshots or screencasts are attached as necessary
  • "Ready for review" label attached and reviewers assigned
  • Changes have been reviewed by at least one other contributor
  • Pull request linked to task tracker where applicable

Co-authored-by: Copilot <copilot@github.com>
generateBoxShadowStyleObject(layer.shadows, layerType, context)
);

// Legacy shadow props need multiple layers. Box shadow can do all of them in one, but we want to keep the old ones for backwards compatibility
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should the legacy shadow properties come before, so that modern one takes precendence?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

My understanding is that this would have both showing. Do you think having one above the other would be better? I think that makes sense, but just making sure I'm tracking.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Showing both in the code output is OK. I first assumed shadows in React Native would work like how CSS properties work for standardized and vendor prefixed proprerties (i.e. the last written one takes precedence). However, if the system prioritizes the modern one over the legacy one no matter in which order they're written, this should be fine too.

I guess it wouldn't hurt to output the modern one below the legacy one though, your call. Just let me know, and we can merge it afterwards.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I think I prefer the modern at the bottom, as the shadow box adds multiple properties, while the box shadow adds just one. So it's easier to glance at. Ultimately, I doubt it matters too much, so I'm down for the merge. Thanks for your input

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Box-shadow not supported.

2 participants