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

Remove polymer embeds #1202

Merged
merged 9 commits into from Aug 19, 2019
Merged

Remove polymer embeds #1202

merged 9 commits into from Aug 19, 2019

Conversation

johnpryan
Copy link
Contributor

@johnpryan johnpryan commented Aug 13, 2019

Replaces the embed-*.html files with the experimental ones. This does not make changes to the experimental/ directory yet.

This doesn't tackle #1185, but the websites are already using the experimental embeds so I don't see it as a blocker. Will be following up soon with a fix for that.

After this, the next step will be to update the websites and codelabs to stop using experimental/ and start using these versions, so we can remove experimental/.

closes #892

@johnpryan johnpryan changed the title Replace embed Remove polymer embeds Aug 13, 2019
@johnpryan
Copy link
Contributor Author

johnpryan commented Aug 13, 2019

@devoncarew @domesticmouse @RedBrogdon I think this cleans out most of the polymer files and references, but there might be more. Could you take a look to see if I'm missing anything?

@domesticmouse
Copy link
Member

The bit to confirm is that all the relevant pages still load and render properly. I have a niggling memory that when I attempted to cut polymer out I accidentally broke the main page.

@RedBrogdon
Copy link
Contributor

We also need to look through our analytics data to find out who else is using the embeds and make sure their integrations won't break. We should be able to test them right now by loading the page, altering the iframe URL to point to experiemental with the same query params, and see what happens.

@devoncarew
Copy link
Member

We also need to look through our analytics data to find out who else is using the embeds and make

That makes sense.

You all may already know the main places the embeds are being used. From memory, the main usage was the dartlang web site. cc @kwalrath

It would also be worth getting some page load numbers from Chrome DevTools. When rolling out the previous (polymer based) embed UI, we spent much of our time on the last week or so optimizing the page load time (by asset concatenation at the time, and by tweaking the caching settings in the appengine yaml config file).

@kwalrath
Copy link

I believe that @johnpryan has already taken care of all the embeds in dart.dev.

@johnpryan
Copy link
Contributor Author

It would also be worth getting some page load numbers from Chrome DevTools

That's a good point. We're seeing slower page load times in GA (around 65% slower.) Created #1205 to track.

@johnpryan
Copy link
Contributor Author

@domesticmouse On my machine, index.html is still working as expected.

@RedBrogdon I've looked through the analytics data and can discuss that offline

On the performance side, I did some quick tests to see how the new embed performs. For a brand new page load (empty cache + hard reload) it loads slightly faster (down from 1100ms to 740ms from initial request to the onload event and down 990ms to 578ms to the first meaningful paint).

It's unclear why we're seeing GA show a slower page load, but it looks like analytics.js is loading about 515ms faster on the new embed, so it's possible that analytics.js starts measuring the performance after it loads, so it thinks the new embed is slower.

@RedBrogdon
Copy link
Contributor

I think it would probably be safe to fire this thing off right now, since you've done a lot of work to ensure the old embeds continue functioning. To be sure, though, we should be able to check all these boxes:

  • The new embed supports every query string option that is actually used by an existing embed (e.g. run doesn't count).
  • Our O&O sites have already been updated to the new embed's URL.
  • We've looked through any other pages sending us significant traffic (>100 hits/wk, perhaps) and manually checked that they'll continue working by changing URLs in inspector.

I believe the first two of these are done, and last I heard the conversation was about knocking together a spreadsheet to spread around the work of doing the last one.

@johnpryan
Copy link
Contributor Author

@RedBrogdon done, discussed offline.

Copy link
Contributor

@RedBrogdon RedBrogdon left a comment

Choose a reason for hiding this comment

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

Next up will be to de-experimentalize the new embed.

@johnpryan johnpryan merged commit 1a2d84a into dart-lang:master Aug 19, 2019
@johnpryan johnpryan deleted the replace-embed branch August 20, 2019 19:37
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.

remove polymer from the embedded UIs
6 participants