nandorojo / zeego

Menus for React (Native) done right.
https://zeego.dev
MIT License
1.45k stars 42 forks source link

feat: SFSymbols customization for menu item icons #21

Closed fobos531 closed 1 year ago

fobos531 commented 1 year ago

Resolves #19

As per our conversation, the purpose of this PR is to add extensive customization to SF Symbols provided internally by react-native-ios-context-menu. We're talking icon weight, scale, colors etc. Examples:

CleanShot 2022-09-29 at 22 37 19@2x

CleanShot 2022-09-29 at 22 37 58

I've updated the types as well as the example app. I noticed small bits of repetition in some types so I extracted those as a common type. My only gripe (with TypeScript) is I can't seem to figure out how to properly omit the systemName property from ImageSystemConfig type (see packages/zeego/src/menu/types.ts, L74). Basically we need to remove this since we're passing the icon name through iosIconName, but still have proper autocompletion for the type. I'd appreciate input on that front!

Let me know if you need anything else from me.

vercel[bot] commented 1 year ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
zeego-docs ✅ Ready (Inspect) Visit Preview Oct 17, 2022 at 7:12PM (UTC)
nandorojo commented 1 year ago

Thanks for this! I'm just now seeing it (my notifications on GitHub are crazy rn haha). I'd like to make the following changes:

  1. Make the prop called ios
  2. Deprecate iosIconName in favor of ios.name
  3. We can still fall back to iosIconName, but the TSDoc comment should mark it as deprecated
/**
* @deprecated Please use the `name` inside of the `ios` prop instead.
*/
iosIconName?: string

In v1, I can release this as a breaking change.

fobos531 commented 1 year ago

Hey @nandorojo , thanks for looking at this! I have applied your suggestions, also please see comments made to your review. Let me know if you'd want anything changed.

nandorojo commented 1 year ago

Could you resolve the merge conflicts? Just need to fix the package.json and run yarn. I can merge after that.

fobos531 commented 1 year ago

@nandorojo Good to go!