Skip to content

Conversation

@raquelmu
Copy link

@raquelmu raquelmu commented Mar 6, 2022

Added caretStyle property to the component to allow more customization.

@raquelmu
Copy link
Author

raquelmu commented Mar 6, 2022

I just saw there was another PR with similar work:
#18

@williangaspar
Copy link

@eveningkid This PR would help with my project, I'm sure other people will benefit from this as well

Copy link
Owner

@eveningkid eveningkid left a comment

Choose a reason for hiding this comment

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

Hi @raquelmu and @williangaspar!

I completely missed the notifications and I'm really sorry for the wait!

This is a great initiative, just reviewed your changes to keep everything consistent :)

Feel free to ask any question if you're blocked on one of the suggested changes

Thank you so much for contributing to the project!

Comment on lines 226 to +228
{...sharedPopoverProps}
forceInitialAnimation
caretStyle={caretStyle}
Copy link
Owner

Choose a reason for hiding this comment

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

Could caretStyle be added to sharedPopoverProps instead?

const sharedPopoverProps = {
  animated,
  ...,
  caretPosition,
  caretStyle, // <<

If we don't, caretStyle won't be added to the other instance of Popover on web below :)

- [style](#style)
- [visible (from Popover)](#visible)
- [wrapperStyle](#wrapperStyle)
- [caretStyle](#caretStyle)
Copy link
Owner

Choose a reason for hiding this comment

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

Could this be added alongside caret* props?

Below caretPosition for example!

Please make sure to also change the position of the caretStyle paragraph :)

style?: PopoverProps['style'];
visible?: boolean;
wrapperStyle?: ViewProps['style'];
caretStyle?: ViewProps['style'];
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
caretStyle?: ViewProps['style'];
caretStyle?: PopoverProps['caretStyle'];

forceInitialAnimation = false,
numberOfLines,
visible = true,
caretStyle = null,
Copy link
Owner

Choose a reason for hiding this comment

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

No need to set a default value, undefined will work too:

Suggested change
caretStyle = null,
caretStyle,

forceInitialAnimation?: boolean;
numberOfLines?: number;
visible?: boolean;
caretStyle: ViewProps['style'];
Copy link
Owner

Choose a reason for hiding this comment

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

caretStyle should be optional :)

Suggested change
caretStyle: ViewProps['style'];
caretStyle?: ViewProps['style'];

@finom
Copy link

finom commented Nov 22, 2022

Guys, any news?

@finom
Copy link

finom commented Nov 22, 2022

For people who wants to use the change now you can edit your dependencies and list the PR as a dependency.

"react-native-popable": "eveningkid/react-native-popable#pull/27/head",
caretStyle={{ backgroundColor: 'red', left: -90 }}

@0horaa
Copy link

0horaa commented Apr 2, 2024

i have no doubt that this would be useful for everyone! an interesting point is that, using yarn, i had to reference the pr as follows:

"react-native-popable": "eveningkid/react-native-popable#27/head",

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.

5 participants