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

Add styling props #16

Merged
merged 8 commits into from
Oct 24, 2018
Merged

Add styling props #16

merged 8 commits into from
Oct 24, 2018

Conversation

tafelito
Copy link
Contributor

@tafelito tafelito commented Oct 20, 2018

This PR adds styling props to the container, cropping area and image

close #15

@ValentinH
Copy link
Owner

In order to use those props, you need to pass a style object right? What I had in mind was more something like containerClass, cropAreaClass, etc... Or a classes property that would take an object like this:

{
  container: 'xyz',
  cropArea: 'abc'
}

This would be simpler to integrate with any styling solution, CSS-in-JSS or not.

@tafelito
Copy link
Contributor Author

I don't use classes. In fact I'm using react-native-web so I wouldn't be able to pass a class or a className. But either way, that could be added as well so you could either pass a class or a style and it will work in both cases

What do you think?

@ValentinH
Copy link
Owner

I understand. On the same idea as the one I suggested with classes, could you group all of them under a styles or customStyle property that would take an key/value map of styles?

Also could you please update the ReadMe so other people know how to use this new feature 🙂

@tafelito
Copy link
Contributor Author

tafelito commented Oct 22, 2018

I haven't really used emotion or styled-components so what would be the best way to merge the classes with the default styles in this case?

I guess it would be just passing the className to each element, right?

@ValentinH
Copy link
Owner

ValentinH commented Oct 23, 2018

Yes exactly. :)

In the same way you added style, you could add classes and pass it via the className prop of each element.

src/index.js Outdated
@@ -268,6 +270,7 @@ class Cropper extends React.Component {
transform: `translate(${x}px, ${y}px) scale(${zoom})`,
}}
imageStyle={imageStyle}
className={cropAreaClassName}
Copy link
Owner

Choose a reason for hiding this comment

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

I think it should be imageClassName here 😄

src/index.js Outdated
@@ -279,6 +282,7 @@ class Cropper extends React.Component {
}}
data-testid="cropper"
cropAreaStyle={cropAreaStyle}
className={imageClassName}
Copy link
Owner

Choose a reason for hiding this comment

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

Opposite here: should be cropAreaClassName

src/index.js Outdated Show resolved Hide resolved
src/index.js Outdated Show resolved Hide resolved
ValentinH and others added 2 commits October 23, 2018 12:07
Co-Authored-By: tafelito <tafelito@gmail.com>
Co-Authored-By: tafelito <tafelito@gmail.com>
@ValentinH ValentinH merged commit 41d5e3a into ValentinH:master Oct 24, 2018
@ValentinH
Copy link
Owner

Thanks for this PR and the adjustements while I was a bit slow to answer 👍

@tafelito
Copy link
Contributor Author

@ValentinH, when do you think you'll publish a new version?

@ValentinH
Copy link
Owner

I would like to group this PR for the release: #18

If it's too long, I'll publish tomorrow :)

@ValentinH
Copy link
Owner

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.

Add custom styling
2 participants