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

Add setting hidden attribute back to hide action #7905

Closed
aghassemi opened this issue Mar 1, 2017 · 1 comment
Closed

Add setting hidden attribute back to hide action #7905

aghassemi opened this issue Mar 1, 2017 · 1 comment

Comments

@aghassemi
Copy link
Contributor

#7785 added setting hidden attribute to elements that get hidden by calling the hide action or component.collapse() but #7879 removed it because the change broke some components such as lightbox.

The reason for breakage was that some components assumed collapse() only sets display:none and had code that would only reset display to show.

We should bring back setting hidden attribute to hide action because it is semantic. Solution might be as simple as moving the logic to our toggle helper function (not the toggle action) but requires some manual testing to be sure it won't have side-effects.

@aghassemi
Copy link
Contributor Author

This has been handled already: https://github.com/ampproject/amphtml/blob/master/src/style.js#L230

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants