Skip to content

Adding “Web Development with AMP” Courses to amp.dev#1887

Merged
sebastianbenz merged 130 commits intofuturefrom
feature/courses
Apr 16, 2019
Merged

Adding “Web Development with AMP” Courses to amp.dev#1887
sebastianbenz merged 130 commits intofuturefrom
feature/courses

Conversation

@matthiasrohmer
Copy link
Copy Markdown
Collaborator

For history see #1595.

@googlebot
Copy link
Copy Markdown

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

ℹ️ Googlers: Go here for more info.

@matthiasrohmer
Copy link
Copy Markdown
Collaborator Author

@morsssss I implemented Longos proposed design for the landing page, please have a look. Additionally added the requested breadcrumbs to the course pages.

Copy link
Copy Markdown
Collaborator

@pbakaus pbakaus left a comment

Choose a reason for hiding this comment

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

Mostly some nits, and the images are embarrassingly large and need to be compressed much better. This can happen as a fast follow if necessary to this one. /cc @morsssss


&-list {

@media (min-width: 768px) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nit: In the future, we should probably abstract these breakpoints (the pixel values) into a consistent, shared file (if not done already).

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I was surprised this hadn't been done already... at least, I couldn't find those, and I found lots of references to the likes of 768px.

Maybe it has been done already, somewhere :)

@pbakaus I'm so embarrassed about those large images. Especially after I spent quite a bit of time compressing the images for the e-comm demo. I never checked the images in this PR.
I'll ask the original authors to do it now.

Of course, the lesson is that even sophisticated developers make this mistake. Would we consider adding an automatic warning to the commit process when large images are detected?

Comment thread frontend/templates/views/partials/breadcrumbs.j2
@morsssss
Copy link
Copy Markdown
Collaborator

In the airport, and my build is failing, so I can't see what you've done.

I assume it's good. You can release it!

  File "/Users/morss/Sites/docs/pages/extensions/amp_dev/__init__.py", line 3, in <module>
    from extension import *
  File "/Users/morss/Sites/docs/pages/extensions/amp_dev/extension.py", line 21, in <module>
    from grow import extensions
ImportError: cannot import name extensions
[9869] Failed to execute script grow

@morsssss
Copy link
Copy Markdown
Collaborator

I see now conversation showing what this problem is. Trying to merge, update, etc before my plane takes off. But don't wait for me to release.

@morsssss
Copy link
Copy Markdown
Collaborator

Ok, got it running!

My only comment is that the cheese bikes image next to the "About the Courses" text is incredibly small if the screen is between about 930px wide and 1024px wide.

Also, one of my config files contained an old filepath, so I fixed that.

@morsssss
Copy link
Copy Markdown
Collaborator

@mandarini , Rob, can you confirm in this PR that you're ok with your commits being used in this project... as described above?

@robocel
Copy link
Copy Markdown
Contributor

robocel commented Apr 15, 2019

I am OK with my commits being added to this project.

1 similar comment
@mandarini
Copy link
Copy Markdown
Collaborator

I am OK with my commits being added to this project.

@morsssss
Copy link
Copy Markdown
Collaborator

Yay, @robocel !

Also I just fixed that issue between 930px and 1024px described above. Others might prefer to simply make the image and text not side-by-side at this size, but I personally like this solution :)

@sebastianbenz
Copy link
Copy Markdown
Collaborator

@robocel can you please formally accept the CLA and reply with "I signed it"

@robocel
Copy link
Copy Markdown
Contributor

robocel commented Apr 16, 2019

@sebastianbenz I signed it

@morsssss
Copy link
Copy Markdown
Collaborator

Just confirming here that everyone's signed the CLA!

@matthiasrohmer
Copy link
Copy Markdown
Collaborator Author

Great! Then nothing is holding us back to merge this AFAIK?

If so I'll merge this as soon as I am back in the hotel and prepare a build.

@pbakaus
Copy link
Copy Markdown
Collaborator

pbakaus commented Apr 16, 2019

@googlebot please re-check.

@sebastianbenz
Copy link
Copy Markdown
Collaborator

Merging as @morsssss confirmed that the contributors' company has signed the CLA.

@sebastianbenz sebastianbenz merged commit bd52453 into future Apr 16, 2019
@sebastianbenz sebastianbenz deleted the feature/courses branch April 16, 2019 08:39
@sebastianbenz sebastianbenz restored the feature/courses branch April 16, 2019 08:39
@sebastianbenz sebastianbenz deleted the feature/courses branch April 17, 2019 01:16
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.

8 participants