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

toggleLoading: remove force paramater #27508

Merged
merged 3 commits into from Apr 16, 2020
Merged

Conversation

samouri
Copy link
Member

@samouri samouri commented Mar 31, 2020

summary
The opt_force attribute for custom-element.toggleLoading is used to force an element to reshow its loading indicator even if isLoadingEnabled returns false. This behavior is only used via mutatedAttributesCallback (amp-bind) in amp-list and amp-twitter.

By taking advantage of base-element.isLoadingReused within isLoadingEnabled, I think we can remove the need for a force option.

testing done

  • all unit tests still pass
  • manually check amp-list
  • manually check amp-twitter

@samouri samouri self-assigned this Mar 31, 2020
@samouri samouri force-pushed the remove-force branch 3 times, most recently from a3e7ee5 to 077b7ae Compare April 7, 2020 18:47
@samouri samouri marked this pull request as ready for review April 7, 2020 18:59
@amp-owners-bot amp-owners-bot bot requested a review from wassgha April 7, 2020 18:59
@samouri samouri requested review from dreamofabear and jridgewell and removed request for wassgha April 7, 2020 19:04
src/custom-element.js Outdated Show resolved Hide resolved
src/custom-element.js Show resolved Hide resolved
@dreamofabear
Copy link

Nice find that force in toggleLoading can be subsumed by isLoadingReused.

@dreamofabear
Copy link

Man these codepaths are so messy. Another thing I noticed: we should avoid calling Timer.delay if loading is not enabled. It's kind of expensive (see in profiler).

@samouri
Copy link
Member Author

samouri commented Apr 9, 2020

we should avoid calling Timer.delay if loading is not enabled. It's kind of expensive (see in profiler).

Happy to pursue in a separate PR! Unless you think this PR directly exacerbates the issue.

Copy link

@dreamofabear dreamofabear left a comment

Choose a reason for hiding this comment

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

Thanks for cleaning this up. LGTM as long as we manually verify that amp-list/amp-twitter works as expected.

src/custom-element.js Outdated Show resolved Hide resolved
src/custom-element.js Outdated Show resolved Hide resolved
src/custom-element.js Show resolved Hide resolved
@lgtm-com
Copy link

lgtm-com bot commented Apr 13, 2020

This pull request introduces 1 alert when merging b1fb267 into c21d89a - view on LGTM.com

new alerts:

  • 1 for Syntax error

@lgtm-com
Copy link

lgtm-com bot commented Apr 13, 2020

This pull request introduces 1 alert when merging fd1b388 into 680f8cf - view on LGTM.com

new alerts:

  • 1 for Syntax error

@danielrozenberg danielrozenberg removed their request for review April 14, 2020 22:56
@samouri samouri merged commit cdbb496 into ampproject:master Apr 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants