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

Karolinska Institutet theme #238

Merged
merged 8 commits into from
Dec 23, 2019
Merged

Karolinska Institutet theme #238

merged 8 commits into from
Dec 23, 2019

Conversation

ellessenne
Copy link
Contributor

Hi,
I added a couple of css files for a theme following the Karolinska Institutet visual identity guidelines.
Thanks!

Alessandro

@ellessenne
Copy link
Contributor Author

I am uploading the logo to reference in the css here.
ki_logo_neg

@ellessenne
Copy link
Contributor Author

Here's a preview of the theme:

ezgif com-gif-maker

@tcgriffith
Copy link
Collaborator

Hi, @yihui , I would love to review this PR, in the mean time, could you take a look at #232 and #233 regarding the proposal of separating theme contribution from the main repo?

Comment on lines 1 to 12
body {
font-family: system, -apple-system, ".SFNSText-Regular", "San Francisco", "Roboto", "Segoe UI", "Helvetica Neue", "Lucida Grande", sans-serif;
}

h1, h2, h3, h4, h5, h6 {
font-family: Cambria, "Hoefler Text", Utopia, "Liberation Serif", "Nimbus Roman No9 L Regular", Times, "Times New Roman", serif;
font-weight: 400;
}

.remark-code, .remark-inline-code {
font-family: Consolas, "Andale Mono WT", "Andale Mono", "Lucida Console", "Lucida Sans Typewriter", "DejaVu Sans Mono", "Bitstream Vera Sans Mono", "Liberation Mono", "Nimbus Mono L", Monaco, "Courier New", Courier, monospace;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Do you really need that many fonts?

  • Seems like you are missing the @import call to import the fonts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the font is fine. it doesn't import the fancy online fonts, the code will use whatever is found in the listed fonts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

A valid concern is that the font appearance may not be consistent across different OS/browsers. I would suggest the same as @pat-s mentioned above, using the @import fonts.

An example from @emitanaka : https://github.com/emitanaka/ninja-theme/blob/master/docs/themes/kunoichi/assets/default-fonts.css

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the font is fine. it doesn't import the fancy online fonts, the code will use whatever is found in the listed fonts.

True, this might not be a practical problem but I see possible confusion for users questioning themselves if they need all these fonts to use the theme properly.

I also did that in the past but nowadays only use the fonts I also import 😄 . This makes it more clear what is happening.

Copy link
Collaborator

@tcgriffith tcgriffith left a comment

Choose a reason for hiding this comment

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

Test locally and looks great. Great work on the added demo.

If @ellessenne is ok with the current font setting then I think it's ready to merge.

Comment on lines 1 to 12
body {
font-family: system, -apple-system, ".SFNSText-Regular", "San Francisco", "Roboto", "Segoe UI", "Helvetica Neue", "Lucida Grande", sans-serif;
}

h1, h2, h3, h4, h5, h6 {
font-family: Cambria, "Hoefler Text", Utopia, "Liberation Serif", "Nimbus Roman No9 L Regular", Times, "Times New Roman", serif;
font-weight: 400;
}

.remark-code, .remark-inline-code {
font-family: Consolas, "Andale Mono WT", "Andale Mono", "Lucida Console", "Lucida Sans Typewriter", "DejaVu Sans Mono", "Bitstream Vera Sans Mono", "Liberation Mono", "Nimbus Mono L", Monaco, "Courier New", Courier, monospace;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

A valid concern is that the font appearance may not be consistent across different OS/browsers. I would suggest the same as @pat-s mentioned above, using the @import fonts.

An example from @emitanaka : https://github.com/emitanaka/ninja-theme/blob/master/docs/themes/kunoichi/assets/default-fonts.css

@ellessenne
Copy link
Contributor Author

ellessenne commented Dec 21, 2019

Hi @pat-s and @tcgriffith, thanks for the feedback on this PR!
I used that set of fonts as they consist of font stacks (e.g.here) that aim to look native, but it's true that slides might look different on different OSes.
I will modify the theme and push a couple of updates ASAP!

Edit:
Here's a new preview of the theme:
file

Copy link
Collaborator

@tcgriffith tcgriffith left a comment

Choose a reason for hiding this comment

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

Great work!

@tcgriffith tcgriffith merged commit 377d9d8 into yihui:master Dec 23, 2019
@yihui
Copy link
Owner

yihui commented Dec 23, 2019

Thank you both! I'll try to look at #232 / #233 soon.

yihui pushed a commit to paulklemm/xaringan that referenced this pull request Dec 24, 2019
* Added KI theme

* Expanded title

* <h*> tags have the same weight as body text

* Added link to correct logo

* Updated NEWS

* Simplified font choice (it's now consistent across OSes)

* More minor edits to style

* Updated demo file
@ellessenne ellessenne deleted the ki-theme branch January 20, 2020 16:04
@ellessenne ellessenne mentioned this pull request Jun 23, 2020
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.

4 participants