Tradeshift / tradeshift-ui

Tradeshift UI is a framework-agnostic JavaScript library to help Tradeshift App developers to create cohesive user experiences and to provide reusable UI components.
https://ui.tradeshift.com
Other
33 stars 44 forks source link

[Aside] ts-asidecover resides on the page #88

Closed vvvts closed 7 years ago

vvvts commented 7 years ago

In Tradeshift.Search app

  1. Open offer page
  2. Open collaboration panel with option aside
  3. Click 'back' browser button The div '.ts-asidecover' stays and blocks everything.

aside.zip

wiredearp commented 7 years ago

@vvvts Just to make sure I'm seeing this correctly: When this happens, is there a console warning saying something like ts.ui.AsideSpirit should not be removed from the document while open?

alexanderpisarev commented 7 years ago

@wiredearp Exactly! I did some fast fix on v4 side: https://github.com/Tradeshift/Apps/pull/3843 But I think, that it is the area of responsibility of ts.ui, but not v4.

wiredearp commented 7 years ago

@alexanderpisarev I agree, we just never though about the history scenario before (where it is actually a good user experience to kill the Aside before it's properly closed).

eba-ts commented 7 years ago

The fix @alexanderpisarev did is being rolled back, @RafalRasztemborski has all the details (@alexanderpisarev is on vacation until Jan 9)

zdlm commented 7 years ago

@sqren You can track the issue here

wiredearp commented 7 years ago

@vvvts and @gogozby or @sqren: I have a question. The problem is basically that the Aside disappears but the cover remains. But it appears that our code is already prepared for this scenario: When an open Aside is removed from the document, it notifies the cover; and if it was the last remaining Aside, the cover will be removed. Since this mechanism appears to be working correctly on my machine :tm:, we can speculate that you guys don't actually remove the Aside from the DOM, but instead you ng-hide it with display:none. Is that correct?

sorenlouv commented 7 years ago

we can speculate that you guys don't actually remove the Aside from the DOM, but instead you ng-hide it with display:none. Is that correct?

I just double-checked, and the container holding all the asides (div.panels.Tradeshift.<appName>) is removed from the DOM when navigating away, however its sibling .ts-cover with attribute data-ts-spirit="ts.ui.CoverSpirit" is not.

Is there a mechanism for us to manually hide the cover? I can hook into angular routing, like:

$scope.$on('$routeChangeStart', (next, current) => { 
  closeCover();
});

@wiredearp

wiredearp commented 7 years ago

@sqren In that case, we have a real bug and we will fix it for real. The cover is never supposed to be removed, by the way, it is just supposed to be hidden. Thanks for the feedback.

sorenlouv commented 7 years ago

The cover is never supposed to be removed, by the way, it is just supposed to be hidden

What about an API for hiding it then?

wiredearp commented 7 years ago

This wouldn't make sense once the bug is fixed, because there already is an API for hiding the cover ("close all the Asides to hide the cover'). If you override this manually, you will also ruin it for the API to show the cover ("open an aside to show the cover"). In other words, the cover is bound to the state of Asides and the problem only exists because we are not correctly tracking the state of Asides. You can however hack it for a quick fix and just remove the cover via jQuery or something. A new one will be created in case you launch a new Aside later on. If we decide at some point to allow non-blocking Asides, then an API would make more sense, even though this would probably be configured on the Aside itself.