Shopify / polaris

Shopify’s design system to help us work together to build a great experience for all of our merchants.
https://polaris.shopify.com
Other
5.79k stars 1.17k forks source link

Prevent admin from re-rendering on form update #3575

Closed kyledurand closed 3 years ago

kyledurand commented 3 years ago

We need to prevent action measuring when screen is vertically resized

Screencast

AsgardLeonardo commented 3 years ago

Hi guys, Found this GH via this Slack convo in # design-language. I have a store reporting the same type of "glitch" but in the collections page:

They have some collections with rather long names which seem to be "out of bounds" and on top of the menus, causing the collection title to flicker and not be able to interact with the menus themselves and actually causing the whole store admin to eventually crash (in Chrome) needing to close the tab and every other Shopify tab.

If the New Design Language is disabled from our view, the (old) collection page shows just fine.

So far I have not been able to replicate this on my test store, not even with a super long name (and under the New Design Language)

Leaving another screencast :)

Merchant store collection example Internal ZD Ticket

kyledurand commented 3 years ago

Thanks @AsgardLeonardo I'll make this a priority today. cc @dfmcphee looks like we still have an infinite render issue

kyledurand commented 3 years ago

Some additional context: https://github.com/Shopify/polaris-react/pull/3576#issuecomment-719727459

kyledurand commented 3 years ago

And a bit more context. I found that the re-rendering of page actions on some pages is actually legit.

On the product details page, here, the preview action takes a new action on every change made to the product form. This is a sort of cached preview of anything that's been changed since save.

We'll have to be more explicit about when our actions do the measuring

oldcheddar commented 3 years ago

hey team 👋

I'm just adding two instances for tracking as I see this has already been prioritized 🙌

ZenDesk | Internal

ZenDesk | Internal

Rony-C commented 3 years ago

Hi all,

I see this is closed but the issue is still persisting on the merchants store.

I'm wondering if this is the same issue? It looks the same as the original video linked posted by Kyle

kyledurand commented 3 years ago

Thanks Rony, it was automatically closed when that PR shipped but we're still waiting on the release to hit web. https://github.com/Shopify/web/pull/34757. I'll close this when that's been pushed to production

kyledurand commented 3 years ago

This fix is in production now, feel free to re-open if the bug persists

PaddyMoloney commented 3 years ago

Hey folks, looks like this is still happening on certain stores https://app.shopify.com/services/internal/shops/10195793

kyledurand commented 3 years ago

Thanks Paddy, we had to revert the fix unfortunately. We have another PR up but it won't be shipped til after BFCM

TaylorCyr commented 3 years ago

Hi Kyle. Before the fix was reverted, I also had a merchant mention that the fix left his product title rendering like this:

At the time I was able to replicate, on Chrome. This product:

https://oceanside-photo-telescope.myshopify.com/admin/products/11230504202

mediumjones commented 3 years ago

Hey @kyledurand not sure if this had anything to do with it, but we updated the page title recently to add product status badges to it. I can try to dig up the PR if you think it'll help.

https://github.com/Shopify/web/issues/30082

patricksayegh commented 3 years ago

Hey team! I have a merchant who is having this issue(I am also able to replicate). From what I found(awaiting the merchant's confirmation as well) it only appears to be happening on one of their products. If you duplicate that product in question, the issue no longer persists on the duplicate - which a workaround could be hiding the affected product and have the merchant use the duplicate. Details are as followed:

Internal Product in question Duplicate of that product, that works Video of issue Zendesk ticket Incident Dashboard

I haven't asked the merchant to delete the product in question, but to hide it and use the duplicated one. I will add more products if they have more and if you require so.

Thanks so much!

TaylorCyr commented 3 years ago

Hey @kyledurand, looks like the PR to address this has been in review for a while. Now that its post-holidays, any chance that could get pushed through please?

Fox04k commented 3 years ago

Hi Guys, just adding another example here. I could not replicate this on my side but merchant has sent a vid of it happening to them.

Product Internal ZD Ticket Video

Seems to only be happening on one of their products, awaiting confirmation of this.

kyledurand commented 3 years ago

Thanks folks, we understand the frustration behind this but also want to be careful we don't introduce any other bugs. Hoping to roll out a fix early next week

Drake2300 commented 3 years ago

@kyledurand Just double checking that closing with that commit was intentional, and this should now be resolved?

kyledurand commented 3 years ago

Whoops, thanks Drake. This will be fixed Monday when we ship the Polaris update to web

kyledurand commented 3 years ago

This shipped yesterday and seems to be stable. Merchants affected should only, in the worst case scenario, see one shift of title content when making updates to product pages

i.e https://screenshot.click/screencast_2021-01-14_13-49-52.mp4

This is because the title and secondary actions are competing for space and on a re-render the JS is somewhat competing with css in determining what can / should be rendered. There's no elegant solution to this as this component needs to support long titles with a few secondary actions, or short titles with lots of secondary actions and we need to maximize what gets shown here.

Feel free to reopen if anything else gets weird