-
Notifications
You must be signed in to change notification settings - Fork 4
feat: Add eLife styling for publication date #114
feat: Add eLife styling for publication date #114
Conversation
@@ -0,0 +1,3 @@ | |||
:root { | |||
--text-size-small: 0.6875; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For clarity should include rem
in the name as the unit is not in the value.
src/themes/elife/styles.css
Outdated
@@ -13,6 +13,8 @@ | |||
@import '../../fonts/notoSerif/notoSerif.css'; | |||
@import '../../fonts/notoSans/notoSans.css'; | |||
|
|||
@import 'custom-properties.css'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move down to be below any @import
s from outside the eLife theme.
@@ -0,0 +1,3 @@ | |||
:root { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use :--root
to be consistent with the rest of the codebase. Might need to change it later to widen the scope to this, but for now let's start with consistency.
src/themes/elife/styles.css
Outdated
font-family: 'Noto Sans', Arial, Helvetica, sans-serif; | ||
font-size: calc(var(--text-size-small) * 1rem); | ||
line-height: calc(1.5rem / var(--text-size-small)); | ||
letter-spacing: 0.5px; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure whether this should be a custom property 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could be as all the letter-spacing variables in pattern library are the same. So will add.
Codecov Report
@@ Coverage Diff @@
## next #114 +/- ##
=======================================
Coverage 88.95% 88.95%
=======================================
Files 4 4
Lines 163 163
Branches 50 50
=======================================
Hits 145 145
Misses 18 18 Continue to review full report at Codecov.
|
src/themes/elife/styles.css
Outdated
@@ -15,6 +15,9 @@ | |||
|
|||
@import '../../extensions/person/styles.css'; | |||
|
|||
/* Custom Properties */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments are good, but in this case the file name is self-documenting so don't need this here.
src/themes/elife/styles.css
Outdated
:--datePublished { | ||
font-family: 'Noto Sans', Arial, Helvetica, sans-serif; | ||
font-size: calc(var(--text-size-rem-small) * 1rem); | ||
line-height: calc(1.5rem / var(--text-size-rem-small)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should probably have the 1.5rem
in a custom property too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidcmoulton What should we call this then? Are you sure this is not fragmenting this too much?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cannot think of a suitable name for this if we are going to use it as a global. I have set it up as if we were going to use it for line-height.
@discodavey In your commit messages, please can you add |
@davidcmoulton It maybe worth putting the info about naming conventions for commits in the README file as it's not obvious and would be good information for anyone else who works on the project. |
This was suggested here: #114 (comment)
:--root { | ||
--text-size-rem-small: 0.6875; | ||
--letter-spacing: 0.5px; | ||
--line-height-calc: 1.5rem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This represents the standard baseline grid measure on eLife (24px), expressed in rem, so would name it something like --baseline-measure-rem
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking okay 👍
Next I guess it's the colour and horizontal alignment?
Also, to get the date formatting as it should you'll need to merge the current next
branch into your feature branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking pretty good 👍
While you're at it, can you get the spacing between the author list and the date the same as eLife too? It's the equivalent of 24px on an eLife article.
/* Colors */ | ||
--color-text-secondary: #888; | ||
|
||
/* Typography */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably don't need these comments (here and on line 2) as it seems pretty well self documenting at the moment. Note that the baseline custom property is not typography but spacing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
With the comments I was just bringing them over from the Pattern Librtary hence why they are there. In my opinion I think they are valid as it's easier to scan comments quicker rather than looking at the code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@davidcmoulton With the spacing on the author list does this need to be a separate PR as the spacing would be on the bottom of the Author List rather than the top of the date?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@discodavey Happy to have a separate PR as part of the same ticket.
Merged into @discodavey can you raise a new PR from your feature branch into @alex-ketch Presumably open PRs currently targeting |
Hi @davidcmoulton just saw the bot notif about this.
Yes on both counts IMO. |
Confirming open eLife PRs are now against |
🎉 This PR is included in version 2.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
First pass for initial styling