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

Shade variation #41

Merged
merged 6 commits into from
Jul 20, 2017
Merged

Conversation

DigitalKrony
Copy link
Contributor

Ability to adjust the shade of the color based on a % or a blend of a desired color variable. Works in both +/- for light and dark colors.

Adam Sivins added 2 commits June 10, 2015 17:44
… desired color variable. Works in both +/- for light and dark colors.
Conflicts:
	src/jquery.adaptive-backgrounds.js
@AlfredJKwack
Copy link
Collaborator

Hi,

@briangonzalez Do you have an issue with the feature itself? I'd be open to review this but the current PR is a little daunting as it has far too many lines vs actual changes and conflicts with the current code base. From what I can tell it should not conflict with the existing implementations and thus be transparent for anyone pulling an update.

@DigitalKrony Without presuming on Brian's answer, the approach I would take is for you to pull a more recent build and submit a new PR with just the changes. As it stands now there's conflicts and quite a bit of extraneous stuff.

From a quick scan of the code I would expect to see just changes at:

  • The DEFAULTS, where you add 3 new properties: shadeVariation, shadePercentage, shadeColors
  • The new function shadeRGBColor(color, percent)
  • The new function blendRGBColors(c0, c1, p)
  • The new function shadeAdjustment(color)
  • The implementation through the if( opts.shadeVariation != false ) {} function.

A clear explanation of shadeRGBColor and blendRGBColors would also help the review as they're a little terse.

Beyond this I really appreciate that you did post a demo.

Once reviewed and accepted by @briangonzalez we'll need to make sure it's documented in the README.md file.

Rgds,
AJK

@DigitalKrony
Copy link
Contributor Author

Well. Cool. Glad you took a look.

It's been sitting around for two years so I would imagine that there'd be some conflicts. I'm booked with work but when I have an opportunity to re-sync with things I'll take a peek and try to remember where I was, though process wise, when I wrote this up.

Watch for my commit.

@AlfredJKwack
Copy link
Collaborator

Will do. Thanks for sticking around.

@briangonzalez
Copy link
Owner

I really like this from a high level. This repo has sort of fallen on disrepair. I am revamping all of my open source projects right now, and this one is top of the list.

With that said, @AlfredJKwack, I trust you to administer changes like this in the interim.

@DigitalKrony – I am very sorry for letting this slip for 2 years! That's my bad. Thank you for the PR.

Copy link
Contributor Author

@DigitalKrony DigitalKrony left a comment

Choose a reason for hiding this comment

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

Long overdue update.

@briangonzalez
Copy link
Owner

@AlfredJKwack What do you think? Shall we?

Copy link
Collaborator

@AlfredJKwack AlfredJKwack left a comment

Choose a reason for hiding this comment

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

Please replace the 4 space indent with 2 space indents on "src/jquery.adaptive-backgrounds.js". Beyond that the changes look good.

@DigitalKrony
Copy link
Contributor Author

DigitalKrony commented Jul 19, 2017

Spaces updated.

Copy link
Collaborator

@AlfredJKwack AlfredJKwack left a comment

Choose a reason for hiding this comment

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

Great!

@AlfredJKwack AlfredJKwack merged commit aad720a into briangonzalez:master Jul 20, 2017
AlfredJKwack added a commit that referenced this pull request Jul 22, 2017
* Update Readme.MD for shade variations features added in PR #41 
* Added AJK as collaborator
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

3 participants