Issue #10 - Content Styles #31
Conversation
There was a problem hiding this comment.
@naarrek Thanks for the PR.
I have added comments, some of them we have already discussed in the chat, the others might be new to you.
As mentioned in the chat there are some inconsitencies with the Drupal version of the website, some inconsistencies seem to be intentational, the other might be not. Guess after the comments will be addressed things might be more clear for us.
There was a problem hiding this comment.
@naarrek Jan thanks for the PR.
I have made only 1 detail comment and some guides for fonts implementation, if you decide implement fonts separately I'm happy to approve this PR (ideally if/when detail comment is also addressed).
theme/less/_header.less
Outdated
| @@ -8,15 +8,18 @@ header > p | |||
| color: #7d7d7d; | |||
| font-size: 24px; | |||
| letter-spacing: -1px; | |||
| line-height: 25px; | |||
| margin-bottom: 0; | |||
There was a problem hiding this comment.
Here line-height controls distance between title and subtitle of the site. Also distance between subtitle and navigation I left to control navigation ul margin considering it "more static (not changeable)" than subtitle.
There was a problem hiding this comment.
Narek Jan, I have added couple of suggestions here -> #32.
I think we have discussed during the last meeting that we can try making the implementation more closer to the Drupal page, if you might have a chance, can you please have a look and let me know if you agree with the decisions, you can find list of adjustments in the PR description.
theme/less/_navigation.less
Outdated
| { | ||
| position: absolute; | ||
| right: 10px; | ||
| top: 33%; | ||
| padding-right: 10px; |
There was a problem hiding this comment.
Maybe it's right to do here padding: 0 10px 0 0.
theme/less/_navigation.less
Outdated
| @@ -93,13 +95,16 @@ | |||
| #main-navigation li.has-children > a:after | |||
| { | |||
| content: url('/images/arrow_bullet.png'); | |||
| padding-left: 10px; | |||
| padding-left: 11px; | |||
There was a problem hiding this comment.
Maybe it's right to do here padding: 0 0 0 11px.
There was a problem hiding this comment.
It will have same effect. Having padding-left seem to be fine.
There was a problem hiding this comment.
I see you still changed to your suggestion. Anyway it's not a big deal, though I feel more in favor of left padding, if there is no such thing as "padding-end"
theme/less/_navigation.less
Outdated
| { | ||
| position: absolute; | ||
| right: 10px; | ||
| top: 33%; |
There was a problem hiding this comment.
vertical-align:middle or vertical-align:sub doesn't work on #main-navigation ul ul li.has-children a:after because it's positioned absolutely. So I added there top with percentage units.
There was a problem hiding this comment.
Right, what about using:
display: flex;
justify-content: space-between;
On <a> element instead?
There was a problem hiding this comment.
It seems display: inline-block on #main-navigation ul ul a solves this vertical height problem. No need to change line-height for <a> , I guess...
There was a problem hiding this comment.
What about the absolute positioning? Not sure how solution with display: inline-block can solve it, might be I didn't understand your comment. But feel free to apply your proposed changes and we can revisit this.
theme/less/_navigation.less
Outdated
| @@ -93,13 +95,16 @@ | |||
| #main-navigation li.has-children > a:after | |||
| { | |||
| content: url('/images/arrow_bullet.png'); | |||
| padding-left: 10px; | |||
| padding-left: 11px; | |||
| vertical-align: sub; | |||
There was a problem hiding this comment.
This aligns a:after as subscript of the text. Instead of this could be for example vertical-align with negative em or percentage values to align image properly.
There was a problem hiding this comment.
This is how it looks like, on my side.
switching this to vertical-align: middle; seem to fix the arrow to be aligned in the middle of the text.
There was a problem hiding this comment.
Also seems like items without side arrow are missing some paddings.
No description provided.