Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 1 commit into from
Closed

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

wants to merge 1 commit into from

Conversation

romgere
Copy link

@romgere romgere commented Nov 13, 2020

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 🤔 .

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 🤔

@romgere romgere changed the title better handle basic-dropdown parent for testing env (#1166) Better handling basic-dropdown parent for testing env (#1166) Nov 13, 2020
@romgere romgere marked this pull request as ready for review November 13, 2020 10:08
@romgere
Copy link
Author

romgere commented Nov 13, 2020

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 😞

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 🤷‍♂️

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.

Subtletree added a commit to Subtletree/ember-paper that referenced this pull request Dec 13, 2021
Subtletree added a commit to Subtletree/ember-paper that referenced this pull request Dec 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant