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

Add inline image preview to table formatter #1636

Merged
merged 4 commits into from
Feb 7, 2017
Merged

Add inline image preview to table formatter #1636

merged 4 commits into from
Feb 7, 2017

Conversation

ebidel
Copy link
Contributor

@ebidel ebidel commented Feb 4, 2017

R: @patrickhulce all

This adds image previews to the formatter. Clicking the previews opens the full size version.

screen shot 2017-02-03 at 5 21 30 pm

@paulirish
Copy link
Member

Looks very nice. :)

I'd put them in the left column, before the URL. The network panel has icons like that.

@ebidel
Copy link
Contributor Author

ebidel commented Feb 4, 2017

I'd put them in the left column, before the URL. The network panel has icons like that.

Sure, we can do that. It'll require some reworking of the CSS. Right now we make the URL 40% of the content width so you see most of it. But that's probably nicer.

@ebidel
Copy link
Contributor Author

ebidel commented Feb 6, 2017

@paulirish PTAL. Moved the image preview to the left of the URL.

screen shot 2017-02-06 at 12 12 22 pm

Also removed the flexbox stuff on the tables. table-layout: fixed ftw.

@patrickhulce
Copy link
Collaborator

niiiice this looks great

@@ -91,6 +94,11 @@ class Table extends Formatter {
}

switch (key) {
case 'previewUrl':
if (/^image/.test(result.mimeType)) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This feels like we're really doubling down on magic happening in the table formatter/assumptions about other keys that don't seem to have a relationship to the caller. What if preview were an object that containedurl and an optional mimeType? Do we have other use cases for non-image previews yet?

Copy link
Contributor Author

@ebidel ebidel Feb 7, 2017

Choose a reason for hiding this comment

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

Good idea. It is a bit magical.

Done.

Do we have other use cases for non-image previews yet?

Not sure yet, but we can certainly add additional ones now :)

Copy link
Member

@paulirish paulirish left a comment

Choose a reason for hiding this comment

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

love me some table-layout:fixed!

url,
url: URL.getDisplayName(image.url),
previewUrl: image.url,
mimeType: image.mimeType,
Copy link
Member

Choose a reason for hiding this comment

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

instead of mimeType you could use resourceType. The difference is that it'll report Image even if a jpg's mimetype was plaintext.

record.resourceType() == Common.resourceTypes.Image;

but looking closer...
this would have to be done back in the gatherer. and we're already using mimetype to determine png/jpg anyway... So i guess it's moot in this case. Something to remember for the next one. :)

cc @patrickhulce

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah I think a comment like this was made on my 4th network record PR, after this was already done :) Though in this case we specifically needed to check mimetype in the gatherer to avoid GIF/SVG/etc

@ebidel
Copy link
Contributor Author

ebidel commented Feb 7, 2017

PTAL

@paulirish paulirish merged commit 08611ab into master Feb 7, 2017
@paulirish paulirish deleted the images branch February 7, 2017 18:03
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.

None yet

3 participants