search

adobe-photoshop / spaces-design

Adobe Photoshop Design Space
http://adobe-photoshop.github.io/
Other
853 stars 74 forks source link

Use inline SVG icon to avoid flashing #3611

Closed shaoshing closed 8 years ago

shaoshing commented 8 years ago

npm install is required

This PR use inline SVG icon (via WebPack) to avoid icon flashing in the following scenarios:

I observe no impact on the load time or initial display time (time until the layer panel is fully displayed).

Layer Panle Performance Comparision

Before and After has the same performance by comapring the results of the Layer Panel Performance Test

reference inline
Layer selection 22ms 19ms
Collapse AB 45ms 43ms
Expand AB 93ms 94ms

(numers are rendering time only)

designedbyscience commented 8 years ago

Never mind previous comment in this space:

I like this

shaoshing commented 8 years ago

@designedbyscience this PR is working as you suggested. SVGIcon will inline the SVG by default. We can still manage the files in the same way without change, and WebPack will compile the files once they are changed.

shaoshing commented 8 years ago

I saw some icons are a little off their original position. need more work to fix them.

iwehrman commented 8 years ago

I ran npm install and restarted grunt debug but I still get this runtime error in the SVGIcon render method:

Transfer from documents.beforeStartup to documents.initActiveDocument failed: Error: Cannot find module './ico-libraries-cc.svg'.
    at file:///Users/wehrman/Desktop/photoshop-Release_x86_64/Plug-Ins/Spaces/www/build/spaces-design-en.js:33728:42
    at webpackContextResolve (file:///Users/wehrman/Desktop/photoshop-Release_x86_64/Plug-Ins/Spaces/www/build/spaces-design-en.js:33728:90)
    at webpackContext (file:///Users/wehrman/Desktop/photoshop-Release_x86_64/Plug-Ins/Spaces/www/build/spaces-design-en.js:33725:30)
    at React.createClass.render (file:///Users/wehrman/Desktop/photoshop-Release_x86_64/Plug-Ins/Spaces/www/build/spaces-design-en.js:33530:48)
shaoshing commented 8 years ago

@designedbyscience @iwehrman @placegraphichere I've fixed all known style issues due to the change of SVG. I also checked the icons under different font sizes (base-8 and base-11) and they are all displaying correctly. So, this PR is now ready for merging. Since the SVGIcon is now much simpler with a most of the code removed, can you help review my change to make sure that I did not break anything? :)

BTW, the icons are correct without applying the viewBox property, so I am wondering if I should remove the property completely in other components, or leave them unchanged just in case?

iwehrman commented 8 years ago

Everything looks great! Please also take a look @designedbyscience and @placegraphichere, but I'm mergin' away.

placegraphichere commented 8 years ago

I am excited about this