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

feat(ui): non full-windowed support #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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

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

@@ -6,6 +6,10 @@ export interface ExplosionProps {
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?: {

@@ -14,6 +14,10 @@ type Props = {|
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?: {

@@ -178,9 +182,9 @@ class Explosion extends React.PureComponent<Props, State> {
};

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

None yet

3 participants