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

Adding Charmes #208

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

Adding Charmes #208

wants to merge 6 commits into from

Conversation

spencerking
Copy link
Contributor

I had forgotten to branch for my initial README pull request. I would assume this should be able to be merged after my first pull request, but if not I can redo it. I tried to emulate the example image from here.

I was looking at the wrong comparison image initially
@una
Copy link
Owner

una commented Oct 4, 2016

Hey! Do you ming getting this branch up to date today? You can go git pull --rebase origin master and go through the merge conflicts

# Conflicts:
#	site/css/cssgram.min.css
#	site/css/demo-site.css
#	site/css/demo-site.min.css
#	source/css/cssgram.css
#	source/css/cssgram.min.css
#	source/scss/cssgram.scss
@spencerking
Copy link
Contributor Author

I think I took care of it all, let me know if I missed something.

@una
Copy link
Owner

una commented Oct 4, 2016

I think this might benefit from a little more warmth in the highlight and violet in the shadow. What do you think? Also, do you mind committing only the updated scss and data files? (ignoring the built CSS)

@spencerking
Copy link
Contributor Author

I can tweak it a bit tomorrow. It's kinda hard to tell from the sample image but I see where you're coming from. In the future I'll ignore the built CSS.

@spencerking
Copy link
Contributor Author

I'm a little worried the image as a whole might be too violet now. I'm having trouble deciding since I think it looks fine when I move a little further away from my monitor; I figured I'd let someone else take a look at it.

@una
Copy link
Owner

una commented Oct 6, 2016

Here is the code I tried:

@mixin charmes($filters...) {
  @extend %filter-base;
  filter: contrast(1.1);

  &::after {
    background-color: rgba(140, 46, 171, .2);
    mix-blend-mode: lighten;
  }

  &::before {
    background-color: rgba(154, 147, 108, .58);
    mix-blend-mode: darken;
  }

  @content;
}

And it looked pretty close to me:
screen shot 2016-10-05 at 10 43 59 pm
screen shot 2016-10-05 at 10 44 05 pm
screen shot 2016-10-05 at 10 44 10 pm

@spencerking
Copy link
Contributor Author

I think what you used is good but it looks too dark with the example I was using. The left is the original and the right is with the Instagram Charmes filter: http://imgur.com/a/QzavO. I took what you had and went ahead and upped the brightness and contrast a bit. This fixed the example I was using and didn't noticeably change the images you posted.

@una
Copy link
Owner

una commented Nov 2, 2016

Awesome, do you mind resolving the merge conflicts? A way to mitigate them is to only submit the _charmes.scss, cssgram.scss, and updated JSON file.

Heres an example: #210

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.

2 participants