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

ITCSS improvements and automatics style guide generation #112

Merged
merged 36 commits into from Feb 13, 2017

Conversation

luboskmetko
Copy link
Member

This PR is inspired by inuitcss and adds new ITCSS elements, objects, utilities and sample component.

Templates structure is defined in line with ITCSS so it allows automatic style guide creation from existing ITCSS layers. Here is how default style guide looks like http://www.screencast.com/t/sRuAg2pU

This is not fully finished yet (eg. sync with WP starter theme is needed) but shows the way how we could provide sensible defaults for developers. By adding optional components we could for example create a white label WP theme from our starter theme.

@thymikee
Copy link
Contributor

Any reason for duplicating ITCSS group name?

styles/itcss/components/_components.footer.scss

I'd rather stick with current setup:

styles/itcss/components/_footer.scss

@@ -0,0 +1,60 @@
/* ==========================================================================
#BUTTONS
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe let's change this to components/_btn.scss?
I like having the component name corresponding with its filename

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea

@luboskmetko
Copy link
Member Author

@thymikee that's how it's defined in inuitcss, I've tried to follow its conventions as much as possible in case we would decide to integrate it in the future. I know it's a duplication but on the other hand it can make it a bit clearer in which layer you are in. Anyway, I'm ok to remove those if we find them redundant.

@thymikee
Copy link
Contributor

It's unnecessary redundancy IMO, let's get rid of this.

Also for elements/ – I often find myself wondering, if e.g. html is under page.scss or maybe layout.scss or base.scss or something. It's a pain.
Since all we do here is overriding default styles applied by the browser vendor, could we just move all of them into one elements/_index.scss file? Commens seems like a good spearation between.

@luboskmetko
Copy link
Member Author

Ok, I'll do. I will also rename _elements.page.scss to _html.scss, but I'd like to keep elements separated so we don't end up with one big monolithic file.

Btw. I hope that with components based Twig templates we could also allow #72 relatively easily. We could have some Twig function which would output stylesheet link (or just enter it manually at the top of component file) and then generate components CSS files in build.

@thymikee
Copy link
Contributor

Are we going to ship all these components by default? I'd like to opt-in for this actually

@luboskmetko
Copy link
Member Author

The plan is that in frontend projects there will be 3 components: header, footer (minimal ones with just a few lines of HTML/CSS) and a button as a sample component ITCSS component.

In the WP projects there will be components which you need to have a functional base theme - the above plus main nav, sidebar, tease post, comment and comment form. These all will be just with minimal styling but should provide consistent look & feel of theme out of box.

@thymikee
Copy link
Contributor

thymikee commented Feb 8, 2017

This is really big diff!

I like the idea of Style Guide, but I'm not convinced it should always be a part of our clients projects.

Problem 1: Right now all these styles are included in the output stylesheet, and we cannot opt-out easily.
Solution: Create style-guide directory and put its styles right there, so developers can easily opt-out by removing @import 'style-guide/*';

Problem 2: There's a great number of default styles that may never be needed for some devs, so they will need to remove them for every project they make.
Solution: Make extensive default styles optional through CLI option (but leave most common mixins, setup and resets), e.g.

Do you want to include a style guide? (Ny)

As for the Problem 2, there's already a number of styles I'm removing every time, because I like it differently. We should provide a vital minimum of styles, but not more.

@luboskmetko
Copy link
Member Author

Thanks for the comments @thymikee.

I'm wondering what are the reasons for removing styles - to reduce file size if they are not used or because some of those styles affect default styling of elements (eg. lists)? Also can you provide a list of exactly what styles you remove?

The idea here was that developer starts with some sensible defaults and ITCSS objects they could reuse for rapid development instead of starting over and over. So if developer needs a media object, they could use .o-media, etc.

Even if we don't use default styling for HTML elements, there is still some default styling coming from the browser. Then there are a few components (header, footer) so pages and especially WP sites look somehow out of box but those could be easily removed just by deleting those CSS files.

If someone likes to start fresh with completely no styles, we could rather provide an alternative minimal ITCSS version during setup. As you said that one can include reset, setup and mixins and remaining folders could be empty folders. Something like:

Select ITCSS version you'd like to use:

  • Full ITCSS (with default elements styling, objects and sample compoments)
  • Minimal ITCSS (reset, setup and mixins)

I agree that generating style guide itself could be an optional too.

@thymikee
Copy link
Contributor

thymikee commented Feb 8, 2017

Select ITCSS version you'd like to use:
Full ITCSS (with default elements styling, objects and sample compoments)
Minimal ITCSS (reset, setup and mixins)

Yes!

@thymikee
Copy link
Contributor

I think we can merge this now and provide minimal ITCSS setup in a followup PR.

@luboskmetko luboskmetko merged commit 13602e6 into master Feb 13, 2017
@jakub300 jakub300 deleted the itcss-improvements branch November 21, 2021 02:10
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.

None yet

2 participants