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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

support transforms #350

Merged
merged 7 commits into from Sep 20, 2018
Merged

support transforms #350

merged 7 commits into from Sep 20, 2018

Conversation

mathieudutour
Copy link
Collaborator

@mathieudutour mathieudutour commented Jun 2, 2018

fix #14

const Swatch = ({ name, hex }) => (
  <View
    name={`Swatch ${name}`}
    style={{
      height: 96,
      width: 96,
      margin: 4,
      backgroundColor: hex,
      padding: 8,
      transform: 'scale(-1, 1) rotate(30) translate(10, 10)', // <---- 馃挜
    }}
  >
    <Text name="Swatch Name" style={{ color: textColor(hex), fontWeight: 'bold' }}>
      {name}
    </Text>
    <Text name="Swatch Hex" style={{ color: textColor(hex) }}>
      {hex}
    </Text>
  </View>
);

screen shot 2018-06-01 at 21 05 52

Only a few transforms are supported:

  • translations
  • rotations should be ok
  • scales when using 1 or -1
  • a healthy mix of those

3D transforms don't work. There are probably bugs when changing the transform origin.

There are tons of edge cases that I haven't tested.

@weaintplastic
Copy link
Contributor

Hell yeah! Great work @mathieudutour

@jaridmargolin
Copy link
Collaborator

There are tons of edge cases that I haven't tested.

Would you like to see this merged as is (assuming yes as a PR has been opened)? Not sure what type of balance this project wants to strike. Is implementing something better than nothing? Or would we prefer to have a fully baked solution in order to avoid end user confusion?

I can give this a set of eyes from a coding standpoint, but I am probably not the right person to speak on philosophy of what should / should not get merged.

@mathieudutour
Copy link
Collaborator Author

I'm not sure either. I probably won't have time to work on that a lot more (especially because there are tons of edge cases). So I'd say it's easier to get it out there and fix the bug reports afterwards?

@jaridmargolin
Copy link
Collaborator

@ljharb Do you mind weighing in quickly with whether or not you agree with merging changes of this nature (incomplete / not fully tested, but still providing value to a certain subset of users)?

Copy link
Contributor

@thierryc thierryc left a comment

Choose a reason for hiding this comment

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

Works for me. Transformation works well. Nice move. I just get 4 eslint no-console warning.

Copy link
Contributor

@thierryc thierryc left a comment

Choose a reason for hiding this comment

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

I will test it deeper this week.

@thecalvinchan
Copy link
Contributor

this is awesome, any updates on when we can see this merged?

@thecalvinchan
Copy link
Contributor

hmm, just tested for a bit yesterday and today. seems to not be working with rotates. will test more and follow up with more context

@sml782
Copy link

sml782 commented Jul 24, 2018

Has the latest stable version supported transform now

@mathieudutour mathieudutour merged commit e6e485d into master Sep 20, 2018
@mathieudutour mathieudutour deleted the feature/transform branch September 20, 2018 11:45
sml782 added a commit to sml782/react-sketchapp that referenced this pull request Sep 21, 2018
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.

Handle transforms
6 participants