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

[INFRA] In PDF, color every other row in table in light gray fill #794

Merged
merged 6 commits into from
May 14, 2021

Conversation

sappelhoff
Copy link
Member

@sappelhoff sappelhoff commented May 7, 2021

closes #659

This took me way longer than it should have ... there are many challenges with our pdf generation code, but I think I fixed more than I destroyed. Some of the options we were passing previously were never actually passed, and I still need to figure out why.

Anyhow, this is a start:

  • table rows have alternating colors ... BUT the setting that "start at row 1 and color every even one light gray" does not work fully --> no idea why, but it's not too bad visually. (an improvement over status quo, I think)
  • several options that we were previously passing (including color of links) were somehow swallowed without any alarms raised --> this is now fixed, which also results in slightly changed formatting of code sections (improved, IMHO)
  • we started to accidentally include non-spec MD files in the PDF version of the spec, such as the raw pre-github changelog, or the README of the schema sub-folder (intended for development docs, not spec content) --> this is now fixed.

From my side, this is ready to be merged - but a lot remains to be done in the future.

@KirstieJane
Copy link
Member

(Just swinging by here because I bet this was a weirdly hard PR but I think it will be super useful! Thank you 🙏 )

@sappelhoff sappelhoff requested review from Remi-Gau and tsalo May 7, 2021 19:41
@effigies
Copy link
Collaborator

effigies commented May 7, 2021

grayalign

Is it possible to improve the alignment of the gray fill with the text/borders?

Also, I think it would look a little nicer if the header row remained white. In fact, I feel like I would flip it exactly, since there's a line to distinguish the header from the first row, so both could be white.

Copy link
Collaborator

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Definitely an improvement in any case. Thanks!

@sappelhoff
Copy link
Member Author

Also, I think it would look a little nicer if the header row remained white. In fact, I feel like I would flip it exactly, since there's a line to distinguish the header from the first row, so both could be white.

yes, something is fishy but I couldn't figure out the problem. See these two tables on page 99: They have different coloring although according to the setting, they shouldn't have.

Screenshot:

image

@Remi-Gau
Copy link
Collaborator

Also, I think it would look a little nicer if the header row remained white. In fact, I feel like I would flip it exactly, since there's a line to distinguish the header from the first row, so both could be white.

Am I wrong or the first rwo being gray is not consistent from one table to the next?

But as @effigies said, an improvement already so heppy to merge as is and improve later.

Copy link
Collaborator

@Remi-Gau Remi-Gau left a comment

Choose a reason for hiding this comment

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

Good with me

@sappelhoff
Copy link
Member Author

Am I wrong or the first rwo being gray is not consistent from one table to the next?

no, you are correct 😄 this is something I couldn't figure out, and that I gave up after about one hour of screaming: #794 (comment)

@Remi-Gau
Copy link
Collaborator

Am I wrong or the first rwo being gray is not consistent from one table to the next?

no, you are correct smile this is something I couldn't figure out, and that I gave up after about one hour of screaming: #794 (comment)

ONE HOUIR!? I admire your tenacity.

@sappelhoff sappelhoff merged commit 4a42f0b into bids-standard:master May 14, 2021
@sappelhoff sappelhoff deleted the pdf/tables branch May 14, 2021 09:13
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.

Alternating row fills in tables within the PDF build
4 participants