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

provide better defaults for colortheme and two fontsizes #13

Merged
merged 2 commits into from Oct 11, 2018

Conversation

Projects
None yet
3 participants
@eddelbuettel
Owner

eddelbuettel commented Oct 10, 2018

This improves (or so I hope) three different YAML header values:

  • the colour theme become monashwhite unless overridden
  • the default fontsize is 14pt unless overridden (same code as in Presento)
  • the default titlefontsize is 22pt as before (and can be overriden)
    which shortens the YAML header.

I guess toc and compress can/should stay. Defaults for these two are less obvious.

Thumbs up or down, @robjhyndman and @izahn ?

@robjhyndman

This comment has been minimized.

Contributor

robjhyndman commented Oct 10, 2018

I had put these defaults inside the theme file, and they only got over-ridden in the template if the user had specified something in the yaml. Putting them in the template directly is fine, but then we don't need them also specified in the theme file -- only like to lead to confusion.

I'd prefer to include at least colortheme and titlefontsize in the skeleton yaml, so users can easily see how to modify these. fontsize is less important. toc should stay for the same reason. I'm ok with removing compress.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 10, 2018

That's what eg the vignette is for: showing all possible values. The YAML header is not where documentation should be.

I prefer the skeleton (header) to be minimally viable (ie to work with minimal settings). And it wasn't really -- I had started by deleting these values as a test, and the generated pdf just doesn't look right (just try it in the master branch). Whereas now it does (in the PR branch).

@robjhyndman

This comment has been minimized.

Contributor

robjhyndman commented Oct 10, 2018

Fair enough re yaml. But we still need to not include default values in two places.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 10, 2018

I agree that the 'not in two places' is not ideal, but it hardly hurts.

Ie for fontsize we now do (in template.tex):

\documentclass[$if(fontsize)$$fontsize$,$else$14pt, ...

where the else part is now -- and that is better. I'll make the same change to Metropolis and IQSS (checking what their preferred values is). I don't think this was set at all for Monash (just how it isn't for the other two -- yet).

For titlefontsize we now have

$if(titlefontsize)$
  \setbeamerfont{title}{series=\bfseries,parent=structure,size=\fontsize{$titlefontsize$}{32}}
$else$
  \setbeamerfont{title}{series=\bfseries,parent=structure,size=\fontsize{22pt}{32}}
$endif$

and you had a 24 set. Maybe I overlooked that and it worked -- I changed both at the same time.

As for the color theme, the style does indeed set a default, but the wrong one. We could could just remove that line. My preference would be to use template.tex as it is clearer how to get from YAML header to it -- any yaml variable foo becomes $if(foo)$ which is reasonably simple. OTOH those who know how to read .sty files (but how many Rmd users do?) would know where to alter it. But that is hard for a semi-permanent usage solution via binb -- changing YAML, and copying in another titlepage (whose name we may want to make a YAML variable with titlepage.png as fallback) is way easier. At least to me :)

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 10, 2018

Also, pointing to template.tex as an answer to 'what the heck can I customize easily' is a decent strategy (short of the ideal yet utopian "we just document everything and keep it current" which never happens). It is just one file per style, it always exists, and we can tell folks to look for all the $if(foo)$ constructs. Those values are what the corresponding YAML header should support.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 10, 2018

One more though: How about adding a % Also set in template.tex via a parameter adjustable from the YAML header comment to the value in the .sty file?

Another related concern is: what is going to be your master source for the these, and how do we keep them current? "Manually" may work as they are not that big / do not change that often. Thoughts on this for IQSS, @izahn ?

@izahn

This comment has been minimized.

Collaborator

izahn commented Oct 10, 2018

@robjhyndman

This comment has been minimized.

Contributor

robjhyndman commented Oct 10, 2018

Good idea to add the suggested comments to the sty file.

I've actually removed the monash beamer template from my MonashEBSTemplates package, so the binb package is now its master source. That's much easier for me than remembering to make changes twice. I added a comment in the MonashEBSTemplates help files to warn any users.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 10, 2018

Cool. I like that too. And obviously it's "your" theme so I should let you pick defaults. Ok to leave it with monashwhite, toc and compress true (in yaml), 14pt font and whatever the title has (22 or 24, I'll streamline and check if I get to it).

@robjhyndman

This comment has been minimized.

Contributor

robjhyndman commented Oct 10, 2018

I'm ok with those defaults.

@eddelbuettel

This comment has been minimized.

Owner

eddelbuettel commented Oct 11, 2018

Ok -- take a look at the commit 265eca3 I just made: for colortheme and titlefont size, the file template.tex no longer needs else branches as I updated the default to 'monashwhite' (and left what we had there for size). I tested, both work well as defaults and are alterable.

Good to merge? If so I will and also update the GH repo README.md with a Monash demo etc pp.

@robjhyndman

This comment has been minimized.

Contributor

robjhyndman commented Oct 11, 2018

Looks good to me.

@eddelbuettel eddelbuettel merged commit a7e708f into master Oct 11, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment