Skip to content
This repository has been archived by the owner on May 6, 2019. It is now read-only.

CSS-Rewrite: Documentation and component importing #53

Open
wants to merge 9 commits into
base: css-rewrite
Choose a base branch
from

Conversation

ninjaofawesome
Copy link
Contributor

OVERVIEW

In this branch I:

  • updated the readme
  • added pertinent partials to base.scss
  • updated normalize

NOTES

  • I'm not sure we need to import the mixins, as per Quartz's setup. Included but commented
  • I think we could eliminate base.scss in favor of core.scss serving the same purpose, but will do in a separate ticket.

@sebastiansandqvist
Copy link
Contributor

sebastiansandqvist commented Aug 10, 2017

Can you change the base branch from master to css-rewrite? Hard to tell what changed otherwise

@ninjaofawesome ninjaofawesome changed the base branch from master to css-rewrite August 10, 2017 19:53
README.md Outdated
[Component].jsx.html
[Component].test.jsx.html
index.html
base.css # code coverage css
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that you can exclude all the coverage stuff maybe besides index.html. You will never be touching any of this since it's generated, and if nyc which generates the coverage reports changes its implementation, we'd need to change our docs, so this could very quickly be out of date.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool, will remove. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

Whenever you're looking at code coverage, even for a specific component, you'll just open coverage/index.html

@@ -0,0 +1,195 @@
//not sure this is needed
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm weary of copying and pasting many of these from the current quartz. For one, there's not a lot of BEM in here. Secondly, this enforces the kind of thing that often makes components hard to implement, which is code like this:

.form {
  fieldset {
    &.inline {
      label, .inner-button, input {
        display: inline-block;
      }
    }
  }

I'd rather have something like:

.form--thing-foo {
  display: inline-block;
}

Because otherwise it's really inflexible. ie. you must have a .form with a <fieldset class="inline"> containing a <label>.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree, but there's A LOT that's coming from quartz, to bem-ify everything might take awhile. Could we consider this a first pass and refactor those to bem next, or should we prune out what we're not really using presently?

Copy link
Contributor

Choose a reason for hiding this comment

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

Since everything we're doing is incompatible with existing quartz anyway, there's no harm in building up from nothing and tailoring specifically to these components

display: block !important;
}
.inline {
display: inline-block !important;
Copy link
Contributor

@sebastiansandqvist sebastiansandqvist Aug 10, 2017

Choose a reason for hiding this comment

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

Since this .inline exists, that entire code block I commented on above is actually unnecessary. Instead of

+.form {
+  fieldset {
+    &.inline {
+      label, .inner-button, input {
+        display: inline-block;
+      }
+    }
+  }

which only adds inline-block, this class .inline is sufficient. By adding the css piece-by-piece we can get rid of a whole lot of unnecessary rules.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Perhaps we should audit all the files that we should include for now, all of this is from the existing Quartz CSS. :)

Copy link
Contributor

@sebastiansandqvist sebastiansandqvist left a comment

Choose a reason for hiding this comment

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

Looks great! Merge please!

@ninjaofawesome
Copy link
Contributor Author

@sebastiansandqvist I "bemified" text.scss for review, but I think this is now starting to get into scope creep. However, I have some questions/ideas of what we could do, could be an interesting conversation.

@erikmoldovan
Copy link
Contributor

Heyo. What's left to do before merging? @davegonzalez @ninjaofawesome @sebastiansandqvist

@sebastiansandqvist
Copy link
Contributor

A lot! @ninjaofawesome suggested we have a meeting to discuss in person and I agree we should.

@sebastiansandqvist
Copy link
Contributor

sebastiansandqvist commented Aug 15, 2017

Oops, just to clarify, a lot to do on CSS prior to merging into master but this is totally good for the integration branch IMO. @ninjaofawesome feel free to merge to that branch any time!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants