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

move css/ directory from jekyll into assets/ #43

Merged
merged 6 commits into from
Sep 30, 2016

Conversation

ashmaroli
Copy link
Member

now that jekyll supports assets/ dir within theme gems, css/ and its
contents will be housed within minima's own assets/ directory

Ref: jekyll/jekyll#5402

@@ -8,7 +8,7 @@

{% assign custom_url = site.url | append: site.baseurl %}
{% assign full_base_url = custom_url | default: site.github.url %}
<link rel="stylesheet" href="{{ "/css/main.css" | prepend: full_base_url }}">
<link rel="stylesheet" href="{{ "/assets/css/main.css" | prepend: full_base_url }}">
Copy link
Member

Choose a reason for hiding this comment

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

Can we keep the permalink of the CSS the same?

Copy link
Member Author

Choose a reason for hiding this comment

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

Are you asking me? or are you requesting me to undo this?

Copy link
Member Author

Choose a reason for hiding this comment

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

either ways, I think the answer is no.. its going to be either assets/css/main.css or assets/main.css if we're to follow Parker's Game Plan for Jekyll 3.3

Copy link
Contributor

Choose a reason for hiding this comment

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

For context, the idea here is that /assets will be a magic folder that will be included in the downstream site, saving the user from needing to create a dedicated scss file. See #16.

For existing users, it should "just work", but will need to require Jekyll ~> 3.3 in order to ensure we're not breaking existing sites.

Copy link
Member

Choose a reason for hiding this comment

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

Can a page in assets/ not have a custom permalink in YAML?

Copy link
Contributor

Choose a reason for hiding this comment

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

assets/css/main.scss

Can we talk through a bit the pros and cons of assets/css/main.scss versus /assets/main.scss? I'm just worried we're over optimizing what is otherwise a simple theme.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm here, if that comment was intended for me..

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm advocating assets/css/main.scss, so the following is in context to that:

pros

  • this is a breaking change (to me, at least). I feel, its the right time to initiate the practice of having delegated folders. css for stylesheets, js for scripts, fonts for well.. fonts.
  • having this set right now will show users/developers that Jekyll 3.3 supports delegated folders. minima is not just a theme, its also a boilerplate / reference for future theme gems.

cons

  • having to create two folders to house one file, while preparing to override default settings.
  • (or), in the case of existing sites, having to create assets/ and move their css/ to it, instead of simply renaming css/ -> assets/

Copy link
Contributor

Choose a reason for hiding this comment

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

@ashmaroli what's the value of delegated folders? Especially for a small boilerplate site? Anyone sophisticated enough to create a site with multiple javascript or css files will likely be sophisticated enough to not use the default theme. Put another way, if we do this, we have to assume every other theme does the same. Is that the precedence we want to set?

@parkr can the new assets folder support support sub-folders?

Copy link
Member

Choose a reason for hiding this comment

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

@benbalter Yes! It supports sub-folders.

@parkr
Copy link
Member

parkr commented Sep 23, 2016

We also have the option to add a note to spec.post_install_message that describes the changes.

@parkr
Copy link
Member

parkr commented Sep 26, 2016

We also have the option to add a note to spec.post_install_message that describes the changes.

@ashmaroli Does that work for you?

'<minima>/assets/css/main.scss'.

More Information:
https://github.com/jekyll/minima/issues/43
Copy link
Member

Choose a reason for hiding this comment

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

This is great! Should we add something to the README telling folks how they can use their custom CSS instead, namely by moving their css/main.scss to assets/css/main.scss in their site source? Would be good I think.

Copy link
Member

Choose a reason for hiding this comment

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

This can now be changed to https://github.com/jekyll/minima#customization 😄

@ashmaroli
Copy link
Member Author

Should we add something to the README telling folks how they can use their custom CSS

yes, the README will need an update as well.

@parkr
Copy link
Member

parkr commented Sep 27, 2016

yes, the README will need an update as well.

OK great, so let's update the README and set the URL in the postinstall message to point to the specific instructions in the readme.

@parkr parkr added this to the 2.0.0 milestone Sep 27, 2016

You can choose to override the [`_includes/head.html `](_includes/head.html) file to specify a custom style path.
`css/main.scss` has now moved to a new directory, `assets/` within the theme gem itself. To customize that file, you'll have to create an `assets` directory at the root of your site and copy the entire `css/` over to `<your-site>/assets/`.
Copy link
Member

Choose a reason for hiding this comment

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

Thanks! This is a bit confusing to me to read. What do you think about:

The site's default CSS has now moved to a new place within the gem itself, assets/css/main.scss. This is why running jekyll new does not output any CSS to your new site. To override the default CSS, create a file in your site source at assets/css/main.scss and add your custom CSS. If you would like to start with the template file in this repository, copy assets/css/main.scss to your site at <your-site>/assets/css/main.scss and edit away!

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for the input, but I think it was too much info crammed into a few lines.. not good when a confused user checks this section for an answer. So I altered it a bit.

The site's default CSS has now moved to a new place within the gem itself, assets/css/main.scss. To override the default CSS, the file has to exist at your site source. Do either of the following:

  • Create a new instance of main.scss at site source.
    • Create a new directory css at <your-site>/assets/
    • Create a new file main.scss at <your-site>/assets/css/
    • Add the frontmatter dashes, and
    • Add @import "minima";, to <your-site>/assets/main.scss
    • Add your custom CSS.
  • Download the file from this repo
    • Create a new directory css at <your-site>/assets/
    • Create a new file main.scss at <your-site>/assets/css/
    • Copy the contents at assets/css/main.scss onto the main.scss you just created, and edit away!
  • Copy directly from Minima 2.0 gem
    • Go to your local minima gem installation directory ( run bundle show minima to get the path to it ).
    • Copy the css/ folder from there into <your-site>/assets/
    • Change whatever values you want, inside <your-site>/assets/css/main.scss

Copy link
Member

Choose a reason for hiding this comment

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

OK. I'd like to release this today or tomorrow and this PR is the last piece.

@ashmaroli
Copy link
Member Author

I'd like to release this today or tomorrow and this PR is the last piece.

The PR is ready for one final review.
Regarding the release, may I suggest that you release Jekyll 3.3 and Minima 2.0 at the same time, as our templates do not lock minima to a specific major_version.. ?

@ashmaroli
Copy link
Member Author

@jekyll/minima, @jekyll/core: I'm placing this on vote.. till Parker decides to end it..
Pls exercise your vote on this comment itself:
+1 for assets/css/main.scss
-1 for assets/main.scss

Thanks

@Strangehill
Copy link
Contributor

the styles aren't working when I try this on localhost --- not sure whether that's to be expected, will it need 3.3 to work?

@envygeeks
Copy link

From a Jekyll Assets standpoint assets/css/main.css would allow us to jimmy ourselves into Jekyll's place and serve all assets with some magic, making things a bit more magical. But at the end of the day we can do that anyways really. So I don't care either way... just sayin'

@parkr
Copy link
Member

parkr commented Sep 29, 2016

the styles aren't working when I try this on localhost --- not sure whether that's to be expected, will it need 3.3 to work?

@Strangehill Yes, you need Jekyll 3.3. You can run the master branch and it will work.

@ashmaroli
Copy link
Member Author

I've decided that Minima 2.0 shall release with assets/main.scss instead of assets/css/main.scss
It can be changed if required, in a future release.

Copy link
Member

@parkr parkr left a comment

Choose a reason for hiding this comment

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

LGTM.

@parkr parkr changed the title move css/ directory from jekyll move css/ directory from jekyll into assets/ Sep 30, 2016
@parkr
Copy link
Member

parkr commented Sep 30, 2016

@jekyllbot: merge +major

@jekyllbot jekyllbot merged commit 23e95b0 into jekyll:master Sep 30, 2016
jekyllbot added a commit that referenced this pull request Sep 30, 2016
cyncyncyn pushed a commit to cyncyncyn/testing-cms that referenced this pull request Nov 15, 2016
Major change in minima 2.0, it needs jekyll 3.3: jekyll/minima#43
@davidyell
Copy link

This breaks all default Jekyll installs. I've just installed the latest gem and my site built without any css due to the theme looking in assets/main.css instead of css/main.css. I don't know if this is an issue with Jekyll, Octopress or this theme.

@ashmaroli
Copy link
Member Author

Do you have the source code online for us to debug?

@davidyell
Copy link

davidyell commented Nov 15, 2016

@ashmaroli Sure thing. If you mean the repo, https://github.com/davidyell/davidyell.github.com/tree/source

It's easy to replicate.

octopress new OctoTest
cd OctoTest
bundler install
jeykll serve

Hit the url and you get [2016-11-15 16:08:52] ERROR/assets/main.css' not found.`. Which doesn't really create a great 'first contact' experience for new users.

Using octopress (3.0.11)

@ashmaroli
Copy link
Member Author

The Issue here is that you have to use Jekyll 3.3.0 and above to read css in /assets/

@davidyell
Copy link

@ashmaroli Great, thanks. 👍

Might be worth adding that to the readme file for future users. I'd also be tempted to ask the guys over at https://github.com/octopress/octopress to link to this themes repo somewhere in their docs as it took me a while to find the repo.

@ashmaroli
Copy link
Member Author

ashmaroli commented Nov 15, 2016

What's puzzling is that Minima 2.0 is locked to Jekyll 3.3 n above.. How were you able to successfully run Minima 2.0 against Jekyll 3.2.1?? 🤔

Update: v2.0.0 is not locked to Jekyll 3.3

@davidyell
Copy link

That I'm not sure of. I'm a Ruby noob!

@ashmaroli
Copy link
Member Author

@benbalter Minima 2.0.0 got released before #59 was merged. So, while the issue reported above wont occur with the next release, I think, its time we release a new version a.s.a.p..

@envygeeks
Copy link

At the speed of sap.

@parkr
Copy link
Member

parkr commented Nov 15, 2016

2.1.0 released.

@envygeeks If you want a new version of these gems released, please open an issue or reach out to me directly. Belittling us or otherwise insulting the pace of action is hurtful.

@envygeeks
Copy link

That wasn't even my intention so why are you trying to put words in my mouth and make that my intention? If you need to know I was poking fun at the word "a.s.a.p". Not everybody is out to get you and making wild assumptions like what you just did is how most beefs start.

@parkr
Copy link
Member

parkr commented Nov 15, 2016

That wasn't even my intention so why are you trying to put words in my mouth and make that my intention? If you need to know I was poking fun at the word "a.s.a.p". Not everybody is out to get you and making wild assumptions like what you just did is how most beefs start.

@envygeeks I'm sorry – I believed that you were referring to how slowly we were releasing the new version, because we hadn't done it and #59 was merged a few weeks ago. I don't mean to put words in your mouth, but please understand that when you say something like that, it is perfectly reasonable that we will take offense to your words. The way your comment comes off can have just as much of an impact as what you actually write. A.S.A.P doesn't line up to "At the speed of sap" in terms of the initials so I didn't think you meant that. Sorry for the misunderstanding.

@envygeeks
Copy link

It doesn't need to align, fully. The joke is in what "a.s.a.p" implies and what actually happens most of the time.

@jekyll jekyll locked and limited conversation to collaborators Apr 27, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants