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.

Sign up for GitHub

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

Jump to bottom

🐛 Fixes rendering children when using amp-selector 's toggle action #23688

Merged
merged 6 commits into from
Aug 8, 2019
Merged

🐛 Fixes rendering children when using amp-selector 's toggle action #23688

merged 6 commits into from
Aug 8, 2019

Conversation

wassgha
Copy link
Contributor

@wassgha wassgha commented Aug 5, 2019 •
edited

Closes #20640
Closes #23449

@wassgha wassgha requested a review from sparhami August 5, 2019 18:29
sparhami
sparhami suggested changes Aug 5, 2019
View reviewed changes
Copy link

@sparhami sparhami left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to write a test for this?

@cathyxz
Copy link
Contributor

cathyxz commented Aug 5, 2019

Also closes #23449.

@cathyxz cathyxz mentioned this pull request Aug 6, 2019
Closed
@cathyxz
Copy link
Contributor

cathyxz commented Aug 6, 2019

We can fix the two failing tests here by either stubbing the mutateElement functions to be synchronous or to return the mutateElement as a promise and wait for it in the test.

@wassgha wassgha mentioned this pull request Aug 8, 2019
Closed
@wassgha
Copy link
Contributor Author

wassgha commented Aug 8, 2019

Would it be possible to write a test for this?

@sparhami Since I'm having trouble running e2e tests on my laptop I added a TODO and an issue for this (#23820 ). This should be ready to merge without the e2e test once Travis passes.

@sparhami sparhami merged commit 757429b into ampproject:master Aug 8, 2019
@wassgha wassgha mentioned this pull request Aug 14, 2019
Merged
thekorn pushed a commit to edelight/amphtml that referenced this pull request Sep 11, 2019
@wassgha
🐛 Fixes rendering children when using amp-selector 's toggle action (…
bf77e3a 
…ampproject#23688)

* Fixes rendering issue with amp-selector toggle

* Fixes unit test race condition

* Added TOOD for e2e tests

* Added reason for Promise reject
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sparhami sparhami sparhami approved these changes

@cathyxz cathyxz cathyxz approved these changes

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
4 participants
@wassgha @cathyxz @sparhami @googlebot