Skip to content

Conversation

@jhlegarreta
Copy link
Member

Fix Table 2.1 in chapter CONFIGURING AND BUILDING ITK exceeding the
page width.

Fixes #62.

@jhlegarreta
Copy link
Member Author

Not sure if the rendered table will be too small.

If it is, an alternative would be to use a landscape layout on a page of its own. Looks like there are a number of alternatives:

I've tested rotating, pdflscape and lscape in some separete, MWEs and all seem to work well. rotating and lscape just rotate the content, whereas pdflscape rotates the page itself as well, which I ignore whether it is desirable. Finally, rotating requires using the command \begin{sidewaystable} whereas lscape does not (just using \begin{landscape} \end{landscape} around the table). Either are fine for me if the rest of the pages before/after render OK.

thewtex
thewtex previously approved these changes Mar 27, 2018
Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

I think we have to just rotate the content, not the page, so that it can continue to be printed as a book.

@jhlegarreta
Copy link
Member Author

Thanks for the review @thewtex ! Commit 5bf45e4 implements a landscape orientation for the table while keeping the portrait orientation for the page. Let's check whether the output provided by the CI builds is according to the expectations.

@thewtex
Copy link
Member

thewtex commented Mar 28, 2018

👍 Cool, @jhlegarreta !

The build is failing. Do we need \usepackage{rotating}?

@jhlegarreta jhlegarreta force-pushed the ITKSoftwareGuide-62 branch from 5bf45e4 to a212164 Compare March 28, 2018 13:12
@jhlegarreta
Copy link
Member Author

Yes, sorry for the oversight @thewtex. a212164 addresses this. Thanks for the review.

thewtex
thewtex previously approved these changes Mar 28, 2018
@jhlegarreta
Copy link
Member Author

Thanks @thewtex. Nonetheless, I've checked the output and it does not quite look like we expected. I need to investigate further.

@jhlegarreta
Copy link
Member Author

Included the legend in the sidewaystable environment, which I guess we want. Also, since the table is not immediately below the cross-reference, used colon instead of period.

The table overlapping the header is still unsolved. I've been looking for a solution without success. Will try to look better.

@jhlegarreta
Copy link
Member Author

One option that I do not particularly like (since it is a workaround rather than a solution) is to change the font size in the table to small. Another one is customize the header for float-only pages, as suggested in this post. But again, that may have undesired effects on pages that may be difficult to spot in a long document like ours.

@jhlegarreta
Copy link
Member Author

I do not see an explanation to the CI build failure. @thewtex how can I re-run the CI build?

@thewtex
Copy link
Member

thewtex commented Apr 12, 2018

@jhlegarreta When you click the Details link next to ci/circleci: build, there should be a button, Rerun job with SSH. You can click that, and it will restart the job but also provide a temporary ssh command to log into the system, inspect outputs, etc. If the button is not there, please let me know, and we will get you the required permissions.

@jhlegarreta
Copy link
Member Author

@thewtex that's right: I had already navigated to the cicle CI build to inspect the output and tried Rerun job with SSH but it is not a clickable button for my user it appears. I get the You need write permissions to trigger builds message when I hover it.

@thewtex
Copy link
Member

thewtex commented Apr 12, 2018

@jhlegarreta That's interesting -- maybe a CircleCI bug? You are on the ITKSoftwareGuide developers team, which has Admin privileges on the repository.

I added your account to the Collaborators for the repository with Write access. Please try again to see if that does it.

@jhlegarreta
Copy link
Member Author

Sorry, @thewtex , still the same result.

@thewtex
Copy link
Member

thewtex commented Apr 12, 2018

How about after pushing to the branch?

@jhlegarreta
Copy link
Member Author

Will try tomorrow afternoon then. Thanks for the suggestion !

@jhlegarreta
Copy link
Member Author

@thewtex I discovered why: I did not have my GitHub account connected in my circle CI ' Account Integration` settings. Now, I ignore whether just being part of the Admins would have been enough... Sorry Matt. Re-running the job now...

@thewtex
Copy link
Member

thewtex commented Apr 13, 2018

Ah, that was tricky. Thanks for taking a look, @jhlegarreta

@jhlegarreta
Copy link
Member Author

This is weird: checking out the code for the commit hash at issue (caa048f) is failing in circle CI:

fatal: Could not parse object 'caa048fe9d21116f734f4557fcd635a83215cd42'.

No idea why this should be happening.

@thewtex
Copy link
Member

thewtex commented Apr 14, 2018

@jhlegarreta that is odd...

Since caa048fe9d21116f734f4557fcd635a83215cd42 is the commit on this branch, please try changing the content, then git commit --amend so a new commit is created, then force pushing.

@jhlegarreta jhlegarreta force-pushed the ITKSoftwareGuide-62 branch from caa048f to c7bc943 Compare May 5, 2018 14:43
@jhlegarreta
Copy link
Member Author

@thewtex Sorry for the delay 😬 I've just rebased and pushed. Let's see what the CI builds tell, and if we're able to solve this.

@jhlegarreta jhlegarreta force-pushed the ITKSoftwareGuide-62 branch from c7bc943 to f7fb0f6 Compare May 5, 2018 15:54
@jhlegarreta
Copy link
Member Author

Patch set c7bc943 still showed a table stepping into the header. An alternative to make the table smaller is

\usepackage{lscape}
....
\begin{landscape}
\begin{table}
\begin{center}
\resizebox{1.6\textheight}{!}{
\begin{tabular}{c c c c c c c c c c c c c c c c c}
\toprule
...
\end{tabular} }
\end{center}
...

But still the look of a lonely table in a single page did not look nice to me, and hence in f7fb0f6 I opted for coming back to a portrait orientation table but limiting its width. If we are happy with the result, I'll update the commit message and this will be ready to be merged.

jhlegarreta pushed a commit to jhlegarreta/ITKSoftwareGuide that referenced this pull request May 5, 2018
The compiler list time schedule in chapter `Configuring and Building
ITK` was being displayed with an awkward layout.

See for example p.10 in a recent build belonging to PR InsightSoftwareConsortium#64:
https://open.cdash.org/upload/8fc8cf811a130b76f66c2b81a3f3027ac17e3b09/ITKSoftwareGuide-Book1.pdf

As explained in:
https://en.wikibooks.org/wiki/LaTeX/Mathematics
the `\[` is a LaTeX shorthand for a displayed equation environment.

This topic removes the unnecessary backward slash escape character.
@jhlegarreta jhlegarreta force-pushed the ITKSoftwareGuide-62 branch from f7fb0f6 to 4aaf5b3 Compare May 5, 2018 16:38
@jhlegarreta
Copy link
Member Author

The result looks satisfactory to me. Check it at
https://open.cdash.org/upload/c775d762a71fe6cf7f601d62263f8887f39d8140/ITKSoftwareGuide-Book1.pdf

It's just the position that LaTeX chose that makes the compiler list time schedule be broken that is not that nice. I've added a [h] in 4aaf5b3 to see if that forces LaTeX choose a better position and avoids breaking the list.

Updated the commit message.

@jhlegarreta jhlegarreta force-pushed the ITKSoftwareGuide-62 branch from 4aaf5b3 to f4b59ea Compare May 7, 2018 08:35
Fix `Table 2.1` in chapter `CONFIGURING AND BUILDING ITK` exceeding the
page width: restrict its width.

Fixes InsightSoftwareConsortium#62.
@jhlegarreta jhlegarreta force-pushed the ITKSoftwareGuide-62 branch from f4b59ea to 960378f Compare May 7, 2018 11:25
@jhlegarreta
Copy link
Member Author

Looked OK to me but for the legend: I forgot to force its position 😊. It should look OK in 960378f !

@jhlegarreta
Copy link
Member Author

It does look OK to me 👌 now !

Copy link
Member

@thewtex thewtex left a comment

Choose a reason for hiding this comment

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

Way to go, @jhlegarreta !!

@thewtex thewtex merged commit d750e1e into InsightSoftwareConsortium:master May 11, 2018
@jhlegarreta jhlegarreta deleted the ITKSoftwareGuide-62 branch May 12, 2018 11:05
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.

2 participants