Skip to content

Updates demos (closes #61) #62

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

Merged
merged 9 commits into from
Sep 8, 2014
Merged

Updates demos (closes #61) #62

merged 9 commits into from
Sep 8, 2014

Conversation

marcoscaceres
Copy link
Contributor

@yoavweiss and I fixed up the demos to use the modern syntax.

<source media="(min-width: 480px) and (max-width: 639px)"
srcset="http://respimg.cdnconnect.com/basic-implentation/test_landscape_1@2x.jpg">
<source media="(min-width: 640px)"
srcset="http://respimg.cdnconnect.com/basic-implentation/test_landscape_1@4x.jpg">
Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't art direction, right? So should be srcset with 'x' on img?

Copy link
Member

Choose a reason for hiding this comment

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

They are... The file names are misleading. We should change that in a followup PR (need to change it at the CDN though, or abandon the CDN)

Copy link
Contributor

Choose a reason for hiding this comment

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

OK.

Another thing, I'm not sure it's a good idea to write the media queries like that. What if the viewport is 479.5px? It seems better to only use max-width and move the srcset on the third source to img. The "print" source should probably be placed first so that it can actually be selected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is now done.

@marcoscaceres marcoscaceres changed the title Updates demos Updates demos (closes #61) Sep 8, 2014
marcoscaceres added a commit that referenced this pull request Sep 8, 2014
Updates demos (closes #61) 
Merging so we can push this out. We can fix other bugs separately.
@marcoscaceres marcoscaceres merged commit b39d763 into master Sep 8, 2014
@marcoscaceres marcoscaceres deleted the updates-demos branch September 8, 2014 20:21
@zcorpan
Copy link
Contributor

zcorpan commented Sep 8, 2014

Nice work!

@anselmh
Copy link
Member

anselmh commented Sep 9, 2014

Awesome, thanks guys!

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.

4 participants