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

Add picture example. #842

Merged
merged 3 commits into from May 1, 2018

Conversation

Projects
3 participants
@sheeeng
Contributor

sheeeng commented Apr 27, 2018

Fix #743.

@wbamberg

This comment has been minimized.

Member

wbamberg commented Apr 28, 2018

Thanks for your contribution! this is a nice example! I did have some comments:

  • Ideally the rendered HTML in the "output" window all fits without the user needing to scroll. So I think the image should fit in the pane.

  • I don't think you need two <picture> elements - I think one is enough, and using media="(min-width...> is a good choice. But, I think 600px is too narrow. 900 or even 1000 would be better. (At a browser window width of 600px, the interactive example gets so small it's hardly usable).

  • I'd recommend you use real images, just because it will look better. In some of the other examples we've used images from unsplash.com, and acknowledged them in the output pane: https://interactive-examples.mdn.mozilla.net/pages/tabbed/datalist.html

  • So I think you could choose a couple of real images, one which works better in a landscape aspect and one that works better in portrait, and use media="(min-width...> to select one or the other.

  • I don't think you need to blockquote the instructions, you could just use a p. And I would word it something like "Change the browser window width to see the image change.".

  • I don't think you need most of the CSS you are currently using. But I think you might want to have some to make sure the image is appropriately sized for the output.

I hope that helps!

@sheeeng

This comment has been minimized.

Contributor

sheeeng commented Apr 29, 2018

Thank you for the feedback, @wbamberg. Appreciate them very much.
I have updated the example. Please review them.

@wbamberg

This comment has been minimized.

Member

wbamberg commented Apr 30, 2018

This looks great! Thanks for the updates.

I had a few formatting suggestions:

  • it's going to be more readable if you have a line break in the <source> element:
    <source srcset="/media/examples/picture-wide.png"
            media="(min-width: 1000px)">
  • the footer overlaps the image at narrower widths:

screen shot 2018-04-30 at 12 12 16 pm

The simplest fix for this is just to remove the <br/> elements. Also, I don't think you need to link to the unsplash images - AIUI we don't need to have any attribution, but it's polite to do so, but there's no need to link back to either the images or the photographer, and I'm worried that the footer HTML distracts from the main point of the example. Having said that, it would be nice to link to the photographer, so perhaps we could compromise with something like:

<footer>Images by <a href=https://unsplash.com/@davidclode>David Clode</a> on Unsplash</footer>

(I also think you can omit the period "." in the footer.)

But I would also like to know if @schalkneethling has opinions on attribution for Unsplash images. In other cases we haven't included any links (<datalist>, <select>), and it would be good to be consistent.

@schalkneethling

This comment has been minimized.

Collaborator

schalkneethling commented May 1, 2018

@wbamberg @sheeeng The license on Unsplash does not require a link back, merely attribution. I do think it is the right thing to link back to the artist/image.

With that said, I am thinking we should have a standard piece of HTML you can use to add these to an example which is consistent. And with regards to linking, linking to both Unsplash and the creator is not required. Just linking to the creator is sufficient.

Any thoughts @wbamberg

@schalkneethling schalkneethling requested a review from wbamberg May 1, 2018

@schalkneethling schalkneethling added this to To do in Q2 - Sprint 2 via automation May 1, 2018

@schalkneethling schalkneethling added this to the Quarter 2 ~ Sprint 2 milestone May 1, 2018

sheeeng added some commits Apr 27, 2018

@sheeeng

This comment has been minimized.

Contributor

sheeeng commented May 1, 2018

Updated the example with the feedback. Please review them.

@wbamberg

This comment has been minimized.

Member

wbamberg commented May 1, 2018

@schalkneethling , here's what I suggest:

For this PR, let's add a footer like:

<footer>Images by <a href=https://unsplash.com/@davidclode>David Clode</a> on Unsplash</footer>

That means @sheeeng is able to finish this PR without having to wait for us to decide on a general policy.

Separately I'll file an issue to decide what to do about attribution in general, and we'll use the resolution of that to update all relevant examples.

@wbamberg

Thanks for your contribution @sheeeng , this looks great!

@wbamberg wbamberg merged commit 3832b5e into mdn:master May 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

Q2 - Sprint 2 automation moved this from To do to Done May 1, 2018

@welcome

This comment has been minimized.

welcome bot commented May 1, 2018

Congrats on merging your first pull request! 🎉🎉🎉

@sheeeng sheeeng deleted the sheeeng:add-picture-example branch May 1, 2018

wbamberg added a commit to wbamberg/interactive-examples that referenced this pull request May 1, 2018

Merge remote-tracking branch 'upstream/master'
* upstream/master:
  adding thead, tbody, and tfoot examples (mdn#848)
  Add input type=email example (mdn#840)
  Add picture example. (mdn#842)
  Add input-file example (mdn#852)
  Update dependency fs-extra to v6 (mdn#861)
  Add <input type="datetime-local"> example (mdn#838)
  Update dependency fs-extra to v5 (mdn#859)
  Update dependency browserify to v16 (mdn#858)
  Update dependency stylelint to v9.2.0 (mdn#857)
  Reinstate caption in meta.json (mdn#843)
  Update dependency jest-puppeteer to v2.4.0 (mdn#855)
  Update dependency puppeteer to v1.3.0 (mdn#856)
  Update dependency http-server to v0.11.1 (mdn#854)
  Update dependency bundlesize to v0.17.0 (mdn#853)
  Pin dependencies (mdn#851)
  Add <hgroup> example (mdn#846)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment