HaikuTeam / animator

Design tool for creating Lottie animations and interactive web components
https://www.haikuanimator.com/
Other
1.51k stars 157 forks source link

feat: warn users when importing large files #1013

Closed roperzh closed 5 years ago

roperzh commented 5 years ago

Summary of changes:

This is just a poor man's way to warn users about performance/crashes implications of huge files, we used this approach:

I also TSifyed some components along the way.

Regressions to look for:

Completed checkin tasks:

roperzh commented 5 years ago

~Marking as draft as I'd like to test/clean the code once more before a review.~

stristr commented 5 years ago

Nice approach and thanks for the TS refactors!

I'm wondering if the 30MB blanket filter is going to overwarn for Sketch files that have some larger raster images embedded while missing other files…

It's actually the size (and number) of the individual SVGs that are expected to cause issues—raster images (now that we have base 64 dump/extract) inflate file size but shouldn't make the app much slower on their own.

I think this is particularly important because a single 2 MB SVG with 1000s of <path /> nodes will bring the entire app to a screeching halt, and these are the most common examples I've seen of this problem. In fact, I think the original file from the ticket against this task was a 2.8M AI file which produced a 1.9M artboard. Am I correct in my understanding that this use case is not addressed in this PR?

roperzh commented 5 years ago

@stristr You are absolutely correct, this will not warn on those scenarios, I was just conveniently limiting myself to what is prompted in the Asana Task.

I think that finding the number and the complexity of elements imported is doable but requires a non-trivial amount of work, which makes me think if it makes sense to do this at all, after all this solution was just briefly discussed in the middle of a team meeting. Do you agree or I'm overestimating what needs to be done?

stristr commented 5 years ago

Yeah—it's also a back end problem and not a front-end problem.

My hunch is that we're not going to catch enough legitimate places where we should warn to justify the blanket filter at 30M…but that the Figma warning is actually helpful. I see one of two paths:

stristr commented 5 years ago

(Note that really, we can't consider the problem solved until we have a true SVG complexity filter.)

roperzh commented 5 years ago

I agree 100%, my hunch will be to ship as-is in order to show less warnings => "scare" less users, since this method isn't reliable, but honestly I'm okay either way!

Do you think that this merits team discussion? if so I'll fill a new task.

stristr commented 5 years ago

Let's put in a new ticket for SVG complexity filter, and ship this as is. Rare false positives is better than a terrible UX in my opinion 👍.

roperzh commented 5 years ago

W00t! thanks for the detailed review 🤗