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

non-working boundaries after 2.10.0 update #102

Open
sentisso opened this issue Mar 4, 2023 · 2 comments
Open

non-working boundaries after 2.10.0 update #102

sentisso opened this issue Mar 4, 2023 · 2 comments
Assignees
Labels
bug Something isn't working

Comments

@sentisso
Copy link
Contributor

sentisso commented Mar 4, 2023

Description

I came across one example in #90 that allowed to limit the boundaries of explosions.

I wanted to reproduce this "feature" in my project and it didn't seem to work. I then noticed that the working example uses version 2.9.0 and my non-working project was using the newest 2.10.2 version.
Then I figured out that some changes made in the 2.10.0 version broke the boundaries setting.

Working boundaries (2.9.0): https://codesandbox.io/s/fireworks-js-launch-2-9-0-5k1ypf?file=/src/index.ts
Non-working boundaries (2.10.0): https://codesandbox.io/s/fireworks-js-launch-2-10-0-zyxj43?file=/src/index.ts

The only difference in the above examples is the package version of fireworks-js in package.json.

Which package are you using?

fireworks-js

fireworks-js version

2.10.0

Browser

any

@sentisso
Copy link
Contributor Author

sentisso commented Mar 4, 2023

Found the bug:

In the constructor of class Fireworks, the canvas is created after the options have been updated, therefore the creation of the canvas overrides the previously set options:

// src/fireworks.ts
this.updateOptions(options)
this.createCanvas(this.target) // <- overrides the boundaries with the current screen size

This commit changed the ordering: feat: options are no longer singleton 9078261

Pushed a PR to fix this ordering bug: #103

@sentisso
Copy link
Contributor Author

sentisso commented Mar 4, 2023

Related issue?

When autoresize is enabled, the custom boundaries get overridden again when the screen size changes, which kind of makes sense, but I think it could be handled in a better way.

// src/fireworks.ts
updateSize({ // <- this is the function that the Resize class executes
    width = this.container.clientWidth,
    height = this.container.clientHeight
}: Partial<Sizes> = {}): void {
   // ...
    this.updateBoundaries({
      ...this.opts.boundaries,
      width, // bye, custom boundaries...
      height
    })
}

The snippet of the source code above makes it impossible to use "custom" boundaries when autoresize is enabled.

Somehing similar to this could "fix" this?

Have the boundaries been set by the user?
  Yes: override them ONLY IF they would overflow the canvas
  No: great, override them with the new width and height

Have the custom boundaries been overridden before and do they no longer overflow the canvas now?
  Yes: revert them to their original value
  No: great, override them with the new width and height

Or just don't ever override custom set boundaries. Do you think, that this is a good idea?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants