Skip to content

Conversation

@bertrand-caron
Copy link

Description

Add support for non-fullscreen confetti. Useful if using a navbar, or any other component that obscures part of the screen. behaviour is backwards compatible (defaults to Dimensions.get('window')).

Result

N/A

Related issues

N/A

* Useful if using a navbar, or any other component that obscurs part of
  the screen.
@bertrand-caron
Copy link
Author

@VincentCATILLON Bumping in case you have not seen this yet and are interested :)

@bertrand-caron
Copy link
Author

@VincentCATILLON Are you still maintaining this repository, or is there a better place to merge this?

@VincentCATILLON
Copy link
Owner

Sorry @bertrand-caron about the delay, I'll make a review asap

|------------------|---------------------------------|--------------------------------------------|----------|--------------------------|
| count | number | items count to display | required | |
| origin | {x: number, y: number} | animation position origin | required | |
| window | {width: number, height: number} | width and height of the animation "window" | | Dimensions.get('window') |
Copy link
Owner

@VincentCATILLON VincentCATILLON Sep 30, 2022

Choose a reason for hiding this comment

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

Suggested change
| window | {width: number, height: number} | width and height of the animation "window" | | Dimensions.get('window') |
| dimensions | {width: number, height: number} | width and height of the animation "window" | | Dimensions.get('window') |

looks more explicit than window

x: number;
y: number
};
window?: {
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
window?: {
dimensions?: {

x: number,
y: number
},
window?: {
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
window?: {
dimensions?: {


render() {
const { origin, fadeOut } = this.props;
const { origin, fadeOut, window } = this.props;
Copy link
Owner

@VincentCATILLON VincentCATILLON Sep 30, 2022

Choose a reason for hiding this comment

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

Suggested change
const { origin, fadeOut, window } = this.props;
const { origin, fadeOut, dimensions = Dimensions.get('window') } = this.props;

const { origin, fadeOut, window } = this.props;
const { items } = this.state;
const { height, width } = Dimensions.get('window');
const { height, width } = window ?? Dimensions.get('window');
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
const { height, width } = window ?? Dimensions.get('window');
const { height, width } = dimensions;

@VincentCATILLON
Copy link
Owner

VincentCATILLON commented Sep 30, 2022

Hi @bertrand-caron

Thanks for your nice contribution

I think the prop should be more explicit (dimensions instead of window which is not really appropriate for container dimensions usage imho), can you adjust and i'll publish it ? :)

@VincentCATILLON VincentCATILLON changed the title Feature: Attempt at adding support for non full-windowed animations feat(ui): non full-windowed support Sep 30, 2022
@buchereli
Copy link

Can this get merged @VincentCATILLON would be helpful

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.

3 participants