Skip to content

Rename color variables #3448

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

Merged
merged 12 commits into from
Apr 1, 2018
Merged

Conversation

nitinprakash96
Copy link
Contributor

@nitinprakash96 nitinprakash96 commented Mar 28, 2018

closes #3435

@nlhkabu
Copy link
Contributor

nlhkabu commented Mar 28, 2018

Thanks @nitinprakash96 - #3433 is now merged - the changes in that PR will affect this PR, so you will need to update your branch with master.

Because of this update , we now also have $brand-color-dark which should be renamed to $primary-color-dark.

Another thing which would be helpful - if you could add "Closes #3435" to the top of your pull request, when we merge it, it will automatically close the corresponding issue.

Thanks for your help on this!

@nlhkabu nlhkabu self-requested a review March 28, 2018 17:53
@nitinprakash96 nitinprakash96 force-pushed the rename-color-variables branch from 029999d to 0bd5927 Compare March 29, 2018 11:21
@nitinprakash96 nitinprakash96 changed the title [WIP]Rename color variables Rename color variables Mar 29, 2018
@brainwane
Copy link
Contributor

Hi, @nitinprakash96! Thanks for updating this! I presume it's now ready to re-review. :) @waseem18 could you give this a look when you have a chance?

$brand-color-soft: lighten($brand-color, 45);
$brand-color-dark: darken($brand-color, 10);
$primary-color-light: lighten($primary-color, 45);
$primary-color-dark: darken($primary-color, 10);

Copy link
Contributor

Choose a reason for hiding this comment

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

could we please check / fix the indentation here? thanks :)

@nlhkabu
Copy link
Contributor

nlhkabu commented Mar 30, 2018

Other than a small indentation issue, the code looks good to me. @waseem18 if you could double check we've not missed anything and that all styles are building as expected, that would be great! Thanks

@waseem18
Copy link
Contributor

LGTM as well 👍

$brand-color: #006dad;
$primary-color: #ffd343;
$primary-color: #006dad;
$highlight-color: #ffd343;
Copy link
Contributor

Choose a reason for hiding this comment

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

There still appears to be an indentation issue here. All variables should line up. Check that you're not using tabs rather than spaces :)

nitinprakash96 and others added 5 commits April 1, 2018 02:45
Previously we sorted by submitted_date, which was troublesome as many JournalEntry objects may be committed in a single transaction, leading to identical timestamps.

SQLAlchemy retains the order of jounal creation, and flushes in order so ordering by submitted_date, id is deterministic and represents the logical order of operations.

Closes pypi#3474, thanks to @anowlcalledjosh for reporting
* Add FAQ entry about requesting new Trove classifiers

* Add issue template
@nitinprakash96 nitinprakash96 force-pushed the rename-color-variables branch from 3505a1a to 085f9f8 Compare March 31, 2018 21:42
@nitinprakash96
Copy link
Contributor Author

Sorry about the commit messages. I somehow messed up the branch confusing it with another. I'll fix them soon.

@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 1, 2018

hi @nitinprakash96 - no need to clean up your commit history. We can squash the commits when we merge :)

This all looks good to me - are you happy for me to merge?

@nitinprakash96
Copy link
Contributor Author

@nlhkabu its all good to go then. :)

@nlhkabu nlhkabu merged commit 149a962 into pypi:master Apr 1, 2018
@nlhkabu
Copy link
Contributor

nlhkabu commented Apr 1, 2018

Thank you @nitinprakash96 !

@brainwane
Copy link
Contributor

@nitinprakash96 Thanks! Let us know if you'd like a suggestion for another issue to work on!

@nitinprakash96
Copy link
Contributor Author

nitinprakash96 commented Apr 1, 2018

@brainwane I'd love to start with some other issue. Could you suggest me some? It'd be gre

@brainwane
Copy link
Contributor

@nitinprakash96 Take a look at #3184? Comment there if you want to try working on it. :)

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.

Rename color variables to make more sense
5 participants