fireworks-js icon indicating copy to clipboard operation
fireworks-js copied to clipboard

non-working boundaries after `2.10.0` update

Open sentisso opened this issue 1 year ago • 2 comments

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 avatar Mar 04 '23 11:03 sentisso

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 90782619e4caf4de6f13d53c4d88750cf0540216

Pushed a PR to fix this ordering bug: #103

sentisso avatar Mar 04 '23 14:03 sentisso

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?

sentisso avatar Mar 04 '23 17:03 sentisso