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

Remove a truckload of wasteful CSS #266

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

lachlanjc
Copy link
Contributor

I've searched the entire app for every class I've removed, and almost all the classes weren't being used at all. Lots more coming.

@@ -120,7 +120,7 @@
list-style: none;
//
&:hover {
background: $black;
background: rgba($black,.5);

Choose a reason for hiding this comment

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

.5 should be written with a leading zero as 0.5
Commas in function arguments should be followed by a single space

Copy link
Contributor Author

Choose a reason for hiding this comment

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

scss-lint says to do the opposite of both of those. Also, I'm hoping the horrible flip grid will be removed soon anyway, so I'm not going to fix this.

@chrislloyd
Copy link
Contributor

Just looking into configuring Hound to Assembly style.

@lachlanjc
Copy link
Contributor Author

@chrislloyd With the semicolon, that's actually necessary. (It's ok since there isn't another @import, but if there was we'd have an error. I'll fix that now.)

But yeah, that'd be awesome.

@chrislloyd
Copy link
Contributor

I just pulled this down and it looks like border colors are off:

iw0ppjw2ykfgeod4uk1ceeaz9hwncvqkjzg4uwnoqp0

Also, it looks like borders on the footer are extending wide:

hjs-fub9s16g2fwzloztcj59ozotglpxaqyj-ukmx_c

and the about page looks a little off:

l2kux0lfkqay_wycsv5nkcsnny-zjf_jp146mcxp8eo

@lachlanjc
Copy link
Contributor Author

Sorry @chrislloyd!

  1. Hmm, ok. I'll take a look at that…not sure what happened.
  2. Crap, got it. I rewrote that but couldn't test it.
  3. Yep. Just merge in the About page refactoring first — I accidentally deleted the styling on this branch as well as on the other. Should I bring it back?

@lachlanjc
Copy link
Contributor Author

@chrislloyd

  1. Just pushed a fix.
  2. Works on my machine :) (it actually does — I'm not seeing that issue at any viewport size, tested on Vivaldi & Chrome)
  3. I've brought it back. There doesn't appear to be any zombie emoji for the occasion, though.

@lachlanjc
Copy link
Contributor Author

It seemed that the owl imports were causing the merge conflicts, but even removing those I can't get things to work. Any other ideas @chrislloyd?

@whatupdave
Copy link
Member

How's this looking? Would love to merge :D

@lachlanjc
Copy link
Contributor Author

Yeah, definitely. How can I figure out what the conflicts are?

@whatupdave
Copy link
Member

Try rebasing this branch css-refactor onto master. Which will be something like:

git pull --rebase upstream master

This should give you a bunch of merge conflicts. Resolve those and then:

git add -A
git rebase --continue

Then force push the branch:

git push -f origin css-refactor

@lachlanjc
Copy link
Contributor Author

@whatupdave I think I’ve fixed it up, thanks for that!

@lachlanjc
Copy link
Contributor Author

@whatupdave @mdeiters @chrislloyd Let’s get this merged :)

@mdeiters
Copy link
Member

mdeiters commented May 8, 2015

@whatupdave is out today but we'll get this merged soon @lachlanjc :D

if (this.isShown && this.options.backdrop)
{
var doAnimate = $.support.transition && animate;

this.$backdrop = $('<div class="modal-backdrop ' + animate + '" />')
.appendTo(document.body);
this.$backdrop = $("<div class='bg-black " + animate + "' />").appendTo(document.body);

Choose a reason for hiding this comment

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

Line is too long.

var transition = $.support.transition && that.$element.hasClass('fade');
this.preloadSize(function() {
that.backdrop(function () {
var transition = $.support.transition && that.$element.hasClass("muted");

Choose a reason for hiding this comment

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

Line is too long.

@@ -99,6 +100,11 @@ let ProductsHandlers = {
type: ActionTypes.PRODUCT_RECEIVE,
product: product
});

Dispatcher.dispatch({

Choose a reason for hiding this comment

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

Expected ',' and instead saw '.'.
Expected ',' and instead saw '('.
Expected an identifier and instead saw '{'.

@lachlanjc
Copy link
Contributor Author

@whatupdave Travis says we’re using a deprecated method for email delivery here: spec/models/wip/worker_spec.rb:36. Could you please take a look at that?

@lachlanjc
Copy link
Contributor Author

Ok, we should be good to go for CSS here.

@lachlanjc
Copy link
Contributor Author

There's one little issue on the padding of bounty lists. I've fixed it but have no Wi-Fi on my 💻. I'll push it up this afternoon (along with another new PR).

@lachlanjc
Copy link
Contributor Author

Ok, issue fixed!

@@ -58,7 +58,7 @@ a.deprecated-card:hover {
padding: 20px;
}

.deprecated-card-footer {
.deprecated-bg-gray-5 py2 px1 {

Choose a reason for hiding this comment

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

Selector should have depth of applicability no greater than 2, but was 3

@include transition-timing-function($transition-function);
}
.deprecated-card-inner {
position: absolute;

Choose a reason for hiding this comment

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

Rule sets should be ordered as follows: @extends, @includes without @content, properties, @includes with @content, nested rule sets
Properties should be ordered background-position, background-size, display, height, opacity, position, width

@lachlanjc lachlanjc changed the title Remove a lot of unused CSS Remove a lot of unused & wasteful CSS Jun 3, 2015
@lachlanjc lachlanjc changed the title Remove a lot of unused & wasteful CSS Remove a truckload of wasteful CSS Jun 6, 2015
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