Skip to content

Conversation

@yoghi
Copy link
Contributor

@yoghi yoghi commented Feb 20, 2018

Now it possible embedded attachment with custom mimeType (ogg, video, pdf, ecc... )

@coveralls
Copy link

coveralls commented Feb 20, 2018

Coverage Status

Coverage increased (+0.09%) to 98.883% when pulling 6a26a3a on yoghi:master into ce9f7b9 on wswebcreation:master.

coverall if/else
@wswebcreation
Copy link
Collaborator

Hi @yoghi

Tnx for the PR, I just released a new version, can you please first merge that one into this PR?

Tnx!

merge with master head
fix wrong merge displayDuration on generation lib
@yoghi
Copy link
Contributor Author

yoghi commented Feb 21, 2018

@wswebcreation merge done.

@wswebcreation
Copy link
Collaborator

@yoghi

Thx, will look at it this week

Copy link
Collaborator

@wswebcreation wswebcreation left a comment

Choose a reason for hiding this comment

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

Awesome addition, tnx!!

Can you also update the readme in the same structure it is now and explain what a user needs to do to get this report.

I also like adding the custom CSS, but maybe we also need to think about a way to make it more easy for people to add custom CSS buy changing some values instead of adding a complete stylesheet. What do you think about that?

<% if (!_.isEmpty(step.attachments)) { %>
<% _.each(step.attachments, function(attachment, attachmentIndex) { %>
<div id="info<%= scenarioIndex %><%= stepIndex %>-attachment<%= attachmentIndex %>" class="scenario-step-collapse collapse">
<object class="attachment" style="height:100%;width:100%;" type="<%= attachment.type %>" id="my_attachments_<%= attachmentIndex %>" data="<%= attachment.data %>">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we do something with this. I don't like the height, I'd prefer it to have the height of the content to get a good overview of the attachment, but I got this

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, with chrome is a problem, with firefox removing style (copy and paste from image, sorry) appeare better, but is not a real solution. I try to search around internet if someone have a solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removing style we have
chrome:
chrome

firefox:
firefox

With embedding data the only solution acceptable seem to be this.
Seem to work well with audio too.

I have try to use iframe instead of object/embed but chrome doesn't like.

I have evaluated as solution use PDF.js but the view increase his complexity and is specific for pdf.

try fix view problem object embedding
split syle: customStyle, overrideStyle
@yoghi
Copy link
Contributor Author

yoghi commented Feb 23, 2018

I have splitted css between "custom" and "override". Custom is append, override is full replace.

update readme
@yoghi
Copy link
Contributor Author

yoghi commented Feb 23, 2018

I haven't add an explain for "multiple attachment" because user no need to do nothing, it's a passive features.

dependency update
@yoghi
Copy link
Contributor Author

yoghi commented Feb 26, 2018

I have update some dependency.

@yoghi
Copy link
Contributor Author

yoghi commented Mar 1, 2018

@wswebcreation missing something?

@wswebcreation
Copy link
Collaborator

Yeah, time, sorry 😢

Hope to do it this weekend and make a release

remove style inline
<% if (step.image) { %>
<div id="info<%= scenarioIndex %><%= stepIndex %>-image" class="scenario-step-collapse collapse">
<a class="screenshot" href="<%= step.image %>" target="_blank">
<img class="screenshot" style="height:100%;width:100%;" id="my_images" src="<%= step.image %>"/>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Removal was not needed because now big images exceed the width of the window. But this triggers me that id="my_images" needs to be replaced with a class because we can have multiple screenshots in a feature

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can add on style.css

.screenshot { width: 100%; height: 100%; }

for multiple screenshot must be change step.image into an array.

@wswebcreation
Copy link
Collaborator

I'll merge the changes tomorrow and add some changes / extra info to the readme. Tnx for the PR and your patience!!

@wswebcreation wswebcreation merged commit 470245e into WasiqB:master Mar 5, 2018
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.

3 participants