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

fix amp-sidebar toolbar issue with chrome #12204

Merged
merged 1 commit into from Nov 28, 2017

Conversation

omar-toma
Copy link
Contributor

@omar-toma omar-toma commented Nov 25, 2017

closes #12205

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

@omar-toma
Copy link
Contributor Author

I signed it!

@googlebot
Copy link

CLAs look good, thanks!

@aghassemi
Copy link
Contributor

aghassemi commented Nov 27, 2017

Thanks for the fix @omar-a-toma
/to @camelburrito for review

toolbar.onLayoutChange(() => this.onToolbarOpen_());
this.getAmpDoc().whenReady().then(() => {
// Check our toolbars for changes
this.toolbars_.forEach(toolbar => {
Copy link
Contributor

Choose a reason for hiding this comment

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

@camelburrito Can this move to buildCallback?

Copy link
Contributor

Choose a reason for hiding this comment

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

No because at this point the toolbar had been built and this code attempts to inject it at the right place on the DOM when layout changes based on the viewport width + toolbar config

Copy link
Contributor

@aghassemi aghassemi left a comment

Choose a reason for hiding this comment

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

LGTM, @camelburrito any changes you like to this?

@ampprojectbot ampprojectbot added this to the Pending Triage milestone Nov 27, 2017
@camelburrito
Copy link
Contributor

I think it i would be better to add one small test to check if we are waiting for onready

@aghassemi aghassemi merged commit 0842729 into ampproject:master Nov 28, 2017
ghost pushed a commit that referenced this pull request Dec 6, 2017
gzgogo pushed a commit to gzgogo/amphtml that referenced this pull request Jan 26, 2018
@omar-toma omar-toma deleted the fix-toolbar-4chrome branch June 19, 2018 11:23
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.

Amp-sidebar toolbar disappear on chrome if the document took too long to load
6 participants