search

miguelcobain / ember-paper

The Ember approach to Material Design.
http://miguelcobain.github.io/ember-paper
MIT License
889 stars 331 forks source link

Better handling basic-dropdown parent for testing env (#1166) #1175

Closed romgere closed 2 years ago

romgere commented 3 years ago

This try to fix Cannot read property 'appendChild' of null issue on paper-menu & paper-select for ember > 3.17 when testing.

I think related issues are :

The code of https://github.com/miguelcobain/ember-paper/blob/b7f13ad6137e456c53afa1bfda8ede0855e1a2d1/addon/utils/ebd-get-parent.js is inspired by what ember-basic-drop-down do : https://github.com/cibernox/ember-basic-dropdown/blob/850c227c0a58148056d55d41aa0e5d88656b8165/addon/components/basic-dropdown.js#L273-L290

In addition to this PR, maybe I miss something, but :

From what I understand here, the reason animateOut exits on paper-select & paper-menu is for animation & to handle isActive :thinking: .

Plus, on paper-select, to keep the scroll position into the drop-down on animate out... (https://github.com/miguelcobain/ember-paper/blob/master/addon/components/paper-select/ebd-content/component.js#L71)

So we duplicate what ember-basic-dropdown already does (https://github.com/cibernox/ember-basic-dropdown/blob/v2.0.15/addon/components/basic-dropdown-content.js#L172-L184) "just" to handle isActive & keep scroll... So we have to fix on ember-paper side an issue already fixed / wich does not exist in ember-basic-dropdown, right ?

Wouldn't it be easier to only handle isActive by our own in paper-select & paper-menu with did-insert & will-destroy, as it is already done, make the scroll behavior "native" in ember-basic-dropdown (seems legit) & rely on ember-basic-dropdown for animation part :thinking:

romgere commented 3 years ago

Wouldn't it be easier to [...] & rely on ember-basic-dropdown for animation part

I made some tests & it seems more complicated than expected... ember-basic-dropdown rely on CSS animation but actually material use CSS transition for select & menu animation :disappointed:

So ember-basic-dropdown "is not able" to wait for animation end before removing the select menu from the dropdown.

I have 0 XP in css transition/animation & don't know if we can override material css for this part or even if it worth it :man_shrugging:

Maybe someone more confident with ember-paper / EBD & css can give some pros/cons about this.

I stop this PR here & wait for some feedback, but I'm still convinced the way ember-paper override dropdown animation for select/menu is not the better way to do & is a source of issue.