added deletion of the success message in the saveGif function

[agrshch]

Resolves #6084

Changes:

Success message after saving a gif with the saveGif() function will not last forever and will disappear in 5 seconds

PR Checklist

• npm run lint passes

[welcome]

🎉 Thanks for opening this pull request! Please check out our contributing guidelines if you haven't already. And be sure to add yourself to the list of contributors on the readme page!

[jesi-rgb]

hey @agrshch! this feels like a lovely detail to add! thanks so much. i actually didn't think of adding this on the first place, but i have some small questions.

when using the function myself, whenever i just downloaded my gif and maybe continued working on it, the message would disappear just because i reloaded the sketch.

i can, for example, imagine a case where a more advanced user just finished a busy and complex animation that takes a lot of time to render. they might go away to do other stuff and hope that the sketch finishes rendering by the time they are back.

in this case, it would be actually useful to leave the message there as a friendly reminder: "hey your sketch finished and downloaded!". on the other hand, making the banner disappear maybe creates confusion as to whether the process actually finished. sure one can check their downloads folder but that is usually not a tidy and clean space haha (mine is definitely not!)

just some perspective as to make this feature as meaningful as possible!

please lemme know your thoughts! @agrshch @Qianqianye

[davepagurek]

@jesi-rgb how would you feel about a little X button at the end of the message to click to dismiss?

[jesi-rgb]

@davepagurek that would probably address all problems! feels like the best compromise between the two.

this also begs the question of whether the pop up is necessary altogether. i believe this little pieces of user feedback are always useful, something that was very much lacking in ccapture and other libraries, for example. they had messages like "be patient, the gif is processing" but that is clearly was not as useful. that's why i added all this little messages!

[agrshch]

Hi @jesi-rgb @davepagurek ,
first of all — the function is amazing! It works super-nice and those messages are cute :)

Currently, I am making a tool for a client. That tool makes graphics, allows users to adjust it and to export the result in several different formats. In this case, a user can make something, then export a gif, look at it, tweak something in settings, export again and so on. It's not really handy to have those notifications on top constantly, and I am afraid it would be too annoying to close them manually each time.

I think my proposal to just delete them automatically 5 seconds after is raw. Instead, those notifications could be adjustable or even optional through the 'settings' object. Also, I've been thinking about a custom callback, that could be passed through the settings object, but this is a separate theme

[jesi-rgb]

@agrshch aha! seeing your particular use case brings a clear purpose.

i believe that the "settings object" proposal also fits within here, being more of an "advance setting" that newcomers should not worry about.

[agrshch]

@jesi-rgb ok, I will try to implement this then.
Btw, do you know why the testing failed? I run the test locally and everything was fine

[jesi-rgb]

thanks so much!! @agrshch please hit us up for any questions :)

about the tests... yeah, they are very finicky. apparently there's an error with gifs not animating properly (you can see that in the build details) but that, i believe, has nothing to do with our side of gif, but rather the function that can import gif to display as images. these two are completely unrelated in the code.

we'll see about that! do not worry too much for now :P

[davepagurek]

@agrshch that's a flaky test case, I'll rerun your tests if I see that one failing again. In the mean time I'll try making a fix in another PR for it :') edit: here it is! #6090

[agrshch]

Hi @jesi-rgb @davepagurek !
I've added three new properties to the settings object. All of them have the default values same as now.
silent, a boolean that defines presence of progress notifications. By default it’s false;
notificationDuration, a number that defines how long in seconds the final notification will live. 0, the default value, means that the notification will never be removed;
notificationID, a string that specifies the notification DOM element id. By default it’s 'progressBar’.

The last one will help to style those notifications if needed.

What do you think?

[jesi-rgb]

I revised all the code and cannot complain! Simple yet effective solution! Great job! 🤩

[agrshch]

@jesi-rgb, thank you!

[davepagurek]

Thanks for making these changes, looks great! My only comments are about matching the code style a little more, but as far as the logic and functionality goes, it looks good to me!

[davepagurek]

Thanks for making those changes!