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

Added _layouts #16

Closed
wants to merge 2 commits into from
Closed

Added _layouts #16

wants to merge 2 commits into from

Conversation

mashsaxena
Copy link

@mashsaxena mashsaxena commented Oct 16, 2019

What type of Pull Request is this?

  • Refactor
  • Bug Fix
  • Feature Enhancement
  • Grammar/Link Fix

Description

Changes proposed in this pull request:

Related Issues and PRs

Fixes #

Mobile & Desktop Screenshots (If any UI changes)

@asdofindia
Copy link
Contributor

Could you describe what you're trying to do? The files you've committed do not look like they're the right ones.

@asdofindia
Copy link
Contributor

Please discuss what you want to do in an issue or at least in the pull request description 🙂 @mashsaxena

@mashsaxena
Copy link
Author

Okay.
Well, I added the _layouts and _include folders because they were not there. :)
If this isn't satisfactory, kindly let me know what I can do and how else should I improve it.
Open to any improvements 💯

@asdofindia
Copy link
Contributor

The folders are fine. But the files in those folders need discuss.

Do we need disqus? Do we need comments at all? In the past comments haven't added value. I personally don't think adding third party content (which invites issues of privacy) is warranted.

I don't think about.md and archive.html belong to _layouts either. Please checkout https://jekyllrb.com/docs/layouts/

There is some related info in #2

We may have to move all the theme related files from minima when we are migrating.

@mashsaxena
Copy link
Author

Okay :)
Will work on it

@Amorpheuz
Copy link
Member

Hey! Thanks for helping out... might I suggest filling in the Description of the PR template with the things you are modifying/adding and reasoning behind them? Also, I would suggest opening Issues for the same before directly going for PR (except its generic grammar/link fixes). This way we can discuss a bit before you tackle the same if required, and not cause confusion about the intent behind the PR. 😄

@cseas
Copy link
Contributor

cseas commented Oct 16, 2019

Hi @mashsaxena, thanks for taking the initiative!
The about and archive pages you've added can be added directly to the site's root directory as suggested in the Jekyll Documentation.

However, it's generally a good practice to add all single pages except index.html to a separate folder called _pages. You can then include the _pages folder into the site by adding the below code to the _config.yml file. Example for reference

include:
- _pages

So, here's what you need to do:

  1. Move the archive and about pages to a separate folder called _pages.
  2. Include _pages folder in _config.yml
  3. Delete the disqus file for now. Comment system can be added later after discussion in Add links to discuss blog posts (github/telegram/etc) #17
  4. Since your archive and about pages refer default layout, a file called default.html is to be added in _layouts. Discussed in Jekyll Migration | Theme fix #2 and Move from gem-based to a regular theme #19

Feel free to comment here for any doubts. Instructions for merging your fixes with the original commit are given in contributing.

@cseas cseas mentioned this pull request Oct 16, 2019
@karx
Copy link
Contributor

karx commented Oct 16, 2019

Needs to be closed or merged with #22

@asdofindia
Copy link
Contributor

I'm gonna close this PR in favour of #22 which is very close to being merged. Please feel free to reopen or create a new PR if #22 doesn't get merged and you have a patch.

@asdofindia asdofindia closed this Oct 18, 2019
@mashsaxena mashsaxena deleted the masoomi branch October 18, 2019 15:05
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.

5 participants