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

Edge Case: _collections bug (solved) #59

Closed
ghost opened this issue Mar 25, 2018 · 6 comments
Closed

Edge Case: _collections bug (solved) #59

ghost opened this issue Mar 25, 2018 · 6 comments

Comments

@ghost
Copy link

ghost commented Mar 25, 2018

So I have been working on adapting this theme to my needs, and I found a slight bug in the theme (or in Jekyll itself, how you like), the issue is that when you create a collection and you use the Boolean output: true which will create a hyperlink/permalink based on title: it also creates an invisible date. That date is then seen by the page-intro.html 'page.date` IF statement and thus all of my collection html pages had an author name, date of creation and read time. Not what I needed.

In theory, there should not be been an IF statement created for the blog meta since there is only one case when you want it to be posted, which is on the articles themselves.

Solving the issues required some working out of the code. I first broke down the page-intro.html into component parts.

  • page-image.html
  • page-meta.html
  • page-introduction.html
  • page-action.html

I did this in order to isolate the page-meta.html.
I then took the common code along with my new pages and added it to default.html in _layouts:

 <header class="intro">

{% include page-image.html %}

<div class="inner">
<div class="intro-text">

{% include page-meta.html %}
{% include page-introduction.html %}
{% include page-action.html %}

</div>
</div>
</header> 

At this point this was my page.meta.html file.

 {% if page.date %}
    {% include author %}
           <p class="entry-meta">
      {% if author_name %}
<span class="byline-item">{{ author_name | prepend: 'by ' }}</span>
{% endif %}
<span class="byline-item"><span class="icon">{% include icon-calendar.svg %}</span><time datetime="{{ page.date | date_to_xmlschema }}">{{ page.date | date: "%B %-d, %Y" }}</time></span>
{% if page.read_time %} <span class="byline-item"><span class="icon">{% include icon-stopwatch.svg %}</span>
{% capture read_time %}{% include read-time.html %}{% endcapture %}{{ read_time | strip }}</span>
{% endif %}
 {% endif %}


After that I needed to create an if statement to only add the meta file if it was a post. So I added this around the include (page.layout == 'post'):

 <header class="intro">
{% include page-image.html %}
<div class="inner">
<div class="intro-text">

{% if page.layout == 'post' %}
{% include page-meta.html %}
{% endif %}

{% include page-introduction.html %}
{% include page-action.html %}

</div>
</div>
</header> 

Which says if this is a post, add this.

The other problem I had was that the CV file has it's own intro.html file which mimics a lot of the page-intro.html file, but changes the title filter to include the json title filter. I ended up removing that file entirely from the control flow and took the parts that were necessary and included them back into the default.html. I mainly took the includes out of the CV.html _layout file like this. heads-up: The {% include cv/intro.html %} was removed.

1 ---
  2 layout: default
  3 ---
  4 {% assign cv = site.data.cv %}
  5 
  6 <main id="main" class="page-content" aria-label="Content">
  7   <div class="index inner">
  8     <div>{{ content }}</div>
  9     <div>
 10       <div class="entries">
 11         {% include cv/basics.html %}
 12         {% include cv/work.html %}
 13         {% include cv/volunteer.html %}
 14         {% include cv/education.html %}
 15         {% include cv/awards.html %}
 16         {% include cv/publications.html %}
 17         {% include cv/skills.html %}
 18         {% include cv/languages.html %}
 19         {% include cv/interests.html %}
 20         {% include cv/references.html %}
 21       </div>
 22     </div>
 23 

I was then able to create an IF statement that would only show the CV title when it was actually the CV page. I also made an if statement for the CV subtitle filter that included an AND statement (see below). I know it seems like a lot, but in the end it works. Here is the new default.html. (one other thing of importance- I needed to re-add the variable {% assign cv = site.data.cv %} to the default.html file above the CV title string for this to work): My new code begins on like 43 just below: <div class="initial-content"> and ends on line 73 in the default.html file.

<!DOCTYPE html>
  2 <!--
  3     Basically Basic Jekyll Theme 1.3.1
  4     Copyright 2017-2018 Michael Rose - mademistakes.com | @mmistakes
  5     Free for personal and commercial use under the MIT license
  6     https://github.com/mmistakes/jekyll-basically-theme/blob/master/LICENSE.md
  7 -->
  8 <html lang="{{ page.lang | default: site.lang | default: 'en-US' }}" class="no-js">
  9   {% include head.html %}
 10 
 11   <body class="layout--{{ page.layout | default: layout.layout }}{% if page.classes or layout.classes %}{{ page.classes | default: layout.classes | join: ' ' | prepend: ' ' }}{%     endif %} {{ page.title | slugify }}">
 12 
 13     {% include skip-links.html %}
 14 
 15     <div class="sidebar-toggle-wrapper">
 16       {% if site.search %}
 17         <button class="search-toggle" type="button">
 18           <svg class="icon" width="16" height="16" xmlns="http://www.w3.org/2000/svg" viewBox="0 0 15.99 16">
 19             <title>{{ site.data.theme.t.search | default: 'Search' }}</title>
 20             <path d="M15.5,13.12L13.19,10.8a1.69,1.69,0,0,0-1.28-.55l-0.06-.06A6.5,6.5,0,0,0,5.77,0,6.5,6.5,0,0,0,2.46,11.59a6.47,6.47,0,0,0,7.74.26l0.05,0.05a1.65,1.65,0,0,0,.5    ,1.24l2.38,2.38A1.68,1.68,0,0,0,15.5,13.12ZM6.4,2A4.41,4.41,0,1,1,2,6.4,4.43,4.43,0,0,1,6.4,2Z" transform="translate(-.01)"></path>
 21           </svg>
 22         </button>
 23       {% endif %}
 24 
 25       <button class="toggle navicon-button larr" type="button">
 26         <span class="toggle-inner">
 27           <span class="sidebar-toggle-label visually-hidden">{{ site.data.theme.t.menu | default: 'Menu' }}</span>
 28           <span class="navicon"></span>
 29         </span>
 30       </button>
 31     </div>
 32 
 33     <div id="sidebar" class="sidebar">
 34       <div class="inner">
 35         {% include navigation.html %}
 36         {% include contact-list.html %}
 37       </div>
 38     </div>
 39 
 40     <div class="canvas">
 41       <div class="wrapper">
 42         {% include masthead.html %}
 43   <div class="initial-content">
 44   <header class="intro">
 45 
 46   {% include page-image.html %}
 47 {% assign cv = site.data.cv %}
 48 
 49   <div class="inner">
 50     <div class="intro-text">
 51         
 52 {% if page.layout == "cv" %}
 53       <h1 class="intro-title">{{ cv.basics.name | default: page.title | markdownify | strip_html }}</h1>
 54 {% else %}      
 55 <h1 class="intro-title">{{ page.alt_title | default: page.title | default: site.title | markdownify | strip_html }}</h1>
 56       {% endif %}
 57 
 58 {% if cv.basics.label and page.layout == "cv" %}
 59         <p class="intro-subtitle">{{ cv.basics.label | markdownify | remove: '<p>' | remove: '</p>' }}</p>
 60       {% endif %}
 61 {% if page.sub_title %}
 62         <p class="intro-subtitle">{{ page.sub_title | markdownify | strip_html }}</p>
 63       {% endif %}
 64 
 65 {% if page.layout == 'post' %}
 66 {% include page-meta.html %}
 67 {% endif %}
 68 
 69 {% include page-introduction.html %}
 70 {% include page-action.html %}
 71     </div>
 72   </div>
 73 </header>
 74   {{ content }}
 75
76    </div>
 77       
 78 
 79         <div class="search-content">
 80           {% include search-form.html %}
 81         </div>
 82       </div>
 83     </div>
 84 
 85     {% include scripts.html %}
 86 
 87   </body>
 88 
 89 </html>

@mmistakes
Copy link
Owner

Since the page meta only shows if page.date is true, did you try taking the simpler route and just add date: false to your collection documents?

You could put it in all of their YAML Front Matter or use front matter defaults in your _config.yml to assign it to all the documents in your collection.

@ghost
Copy link
Author

ghost commented Mar 25, 2018

I thought that there was a setting like that, but I am new to yaml so I wasn't sure and I couldn't find it when I did a lazy look on the internet.
The thing is that I really think that page-intro.html was trying to do too much and made for a lot of repetition and inflexibility. I mean your code is really good as far as it goes, there are just some things that I think could be better.

That's not the main reason I fixed the code, my main reason was the issue with the collections, but there are a few things I'd like to fix.
The only other real thing I want to fix in the code is your inner class. All it really does is put a .5 rem on the element, but you have it being used in multiple (really complex [selector]) ways. And because of that, you have to put the footer inside of the main tag, which isn't semantical. Plus, because you use a different style of inner on the page.html and cv.html, it makes it difficult to just remove the offensive divs and add everything to default.
In my mind the .5 rem should be added on the container and the page should have no inner's whatsoever.
It would be a lot cleaner, more semantical and more flexible.
Don't get me wrong your code is great, but that damn inner crap is frustrating. :)

@mmistakes
Copy link
Owner

The inners are there to add flexibility to the theme as there are cases where content might be null and the layout will look like crap due to inconsistent spacing.

You have valid point about moving footer from main to make it semantically correct. Something I've already taken care of in 6914f7e

@ghost
Copy link
Author

ghost commented Mar 26, 2018

I just figured out that I did not put my code in the right spot, it should be below <div class="initial-content"> if anybody decides to use this code instead of your solution. The one thing I will say is that turning off dates in collections could have adverse affects to your ability to sort the items.

Also, glad you fixed the footer. I will have to look into adding that to my code. But I am still looking to figure out everything with inner. I still think it is too opinionated and makes you have to write too much duplicate code. At the very least, the inner used on the canvas should be renamed so that if we have to keep that that we can at least not be required to include it in our layouts for every single page.

@mmistakes
Copy link
Owner

All my themes are opinionated. They were made for me at some point.

@stale
Copy link

stale bot commented Apr 25, 2018

This issue has been automatically marked as stale because it has not had recent activity.

If this is a bug and you can still reproduce this error on the master branch, please reply with any additional information you have about it in order to keep the issue open.

If this is a feature request, please consider whether it can be accomplished in another way. If it cannot, please elaborate on why it is core to this project and why you feel more than 80% of users would find this beneficial.

This issue will automatically be closed in 7 days if no further activity occurs. Thank you for all your contributions.

@stale stale bot added the Status: Stale label Apr 25, 2018
@stale stale bot closed this as completed May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant