search

fastlane-old / deliver

Upload screenshots, metadata and your app to the App Store using a single command
https://fastlane.tools
2.24k stars 162 forks source link

Fix bug with missing close div tag in html preview #524

Closed cenbarter closed 8 years ago

cenbarter commented 8 years ago

Symptoms

When viewing the summary Preview.html generated by deliver, if you had many languages, each language would appear to have more and more padding applied to the width. This meant that languages that were displayed later in the preview would appear in a much narrower column rather than the full width of the screen. It made them hard to read.

Example

1st language screen shot 2015-12-24 at 3 46 30 pm

14th language screen shot 2015-12-24 at 3 47 29 pm

Fix

There was a missing

tag at the end of the last row of screenshots per language. This meant that the padding from earlier divs was being reapplied for each subsequent language which led to gradually narrower spacing.

As a precaution, a check was added around the last closing div to make sure that we actually added some screenshots.

14th language with the fix applied screen shot 2015-12-24 at 3 48 35 pm

Tests

1) > deliver 2) > deliver --skip_metadata 3) > deliver --skip_screenshots 4) > deliver (deleted screenshot directory so no screenshots to process) 5) > deliver (various combinations of number of languages, devices and screenshots per device 6) verified html no longer has missing div tags with an online html validator

Future suggestions?

1) choose an online html validator 2) fix up current html to pass chosen validator 3) add a test to check results of chosen html validator

KrauseFx commented 8 years ago

Thank you for the pull request and testing it. I added one comment before it's ready to be merged :+1:

cenbarter commented 8 years ago

No problem at all. Thanks for taking the time to look and verify. I've added a response. Happy to discuss further if something isn't clear. Cheers.

lacostej commented 8 years ago

Would the code be easier to read if we were doing something like

<% sc_by_size = sc.group_by {|i| i.screen_size} %>
<% sc_by_size.keys.sort do |screen_size| %>
  <% screenshots = sc_by_size[screen_size].sort {|a, b| [a.path] <=> [b.path]} %>

  <% screenshots.each_with_index do |screenshot, index| %>
    <% if index == 0 %>
    <h4><%= screenshot.formatted_name %></h4>
    <div class="app-screenshot-row">
    <% end %>

    <a href="<%= screenshot.path %>" target="_blank"><img class="app-screenshot" src="<%= screenshot.path %>"></a>

    <% if index == screenshots.length - 1 %>
    </div>
    <% end %>
  <% end %>
<% end %>
KrauseFx commented 8 years ago

Thanks for the pull request, I decided to go with https://github.com/fastlane/deliver/pull/532 for simplicity reasons. Thanks for contributing :+1:

cenbarter commented 8 years ago

Sorry for the delay in responding, I've been away. Yep, totally agree the alternative reads more easily. As this was my first contribution, I just wanted to go with the minimum changes needed to fix the bug. Happy the bug is fixed either way! :)