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

Convert Cart total/subtotal CSS colors to vars #2288

Merged
merged 1 commit into from
Oct 20, 2017

Conversation

gregdaynes
Copy link

This is for issue #2251

Following up on the feedback for #1708, I've taken Martin's suggestion for a variable name, but opted for underscores to match the rest of the variable names.

I've also opted to not use $product_price_text_color for the text - It would make it default to black, which is not inline with the current style, as well is not a fitting name for the context. Instead I've created a new var $cart_total_text_color and assigned it white.

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Thanks, all choices makes sense to me. What about indenting that block of variables with values aligned as the rest of the file?

Copy link
Member

@tvdeyen tvdeyen left a comment

Choose a reason for hiding this comment

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

LGTM.

I think indentation is alright and we should start to not do these things anymore. We don’t want diffs for a huge chunk of code just because we introduced a longer variable name. Rails recently stopped this in schema files as well.

@kennyadsl
Copy link
Member

@tvdeyen I generally agree with not using extra spaces for indentation (I'm also in favor of commas in last element of arrays and hashes to preserve git history). In this case I'd go with being consistent with the rest of the file, since I can't see an easy solution for the current state of the file: I don't think we'll change that file so often to bring it into an ideal indentation state and probably it will be completely changed in the future instead.

Of course this is a non blocking comment and I'm perfectly fine with the current indentation. 👍

@jhawthorn jhawthorn merged commit cf4255b into solidusio:master Oct 20, 2017
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