Fix transforms - missing marginHorizontal marginVertical

[levic92]

marginX and marginY were in the transforms array instead of marginHorizontal and marginVertical, causing negative values to not be processed in the positiveOrNegative function

[vercel[bot]]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/fernandorojo/dripsy/5p1sCGuvRrd8XjNMuiTK8tTCn7zL
✅ Preview: https://dripsy-git-fork-levic92-patch-1-fernandorojo.vercel.app

[levic92]

@nandorojo any chance you've had time to review this bug?

[nandorojo]

why did you remove the existing ones instead of just adding new ones?

[levic92]

The aliases always convert it to either marginHorizontal or marginVertical

[nandorojo]

cool i’ll take a look tomorrow. could you add a test case to the example app?

[levic92]

I could do that but I'd need you to point me in the right direction on where to start. I took a look in the core package and didn't see any tests implemented, and I'm not sure what in the example app you'd want updated

[nandorojo]

it’s just in the examples/with-expo folder

you can edit App.tsx

[levic92]

@nandorojo honestly I'm not sure what you're looking for. There is no with-expo folder that I can see. There is an example folder. I will just give you some code here to demonstrate the bug and hopefully you can merge it to fix the behaviour.

example spacing:

space: {
  "4": 16,
  "5": 20,
  "6": 24,
  "7": 28,
  "8": 32,
}

If you have a these 3 views you'd expect the layout to be the same:

<View sx={{p: 8}}>
  <View style={{backgroundColor: "red", width: "100%", height: 100, marginHorizontal: -16}} />
  <View sx={{backgroundColor: "red", width: "100%", height: 100, mx: -4}} />
  <View sx={{backgroundColor: "red", width: "100%", height: 100, ml: -4, mr: -4}} />
</View>

At the moment the top and bottom view would be the same but the middle one will interpret that as -4 instead of -16

I have also found another bug (related to this same concept) I will leave a second comment below

[levic92]

@nandorojo Second bug occurs if your space values are prefixed with "$"

space: {
  "$4": 16,
  "$5": 20,
  "$6": 24,
  "$7": 28,
  "$8": 32,
}

Same views as previous example:

<View sx={{p: "$8"}}>
  <View style={{backgroundColor: "red", width: "100%", height: 100, marginHorizontal: -16}} />
  <View sx={{backgroundColor: "red", width: "100%", height: 100, mx: "-$4"}} />
  <View sx={{backgroundColor: "red", width: "100%", height: 100, ml: "-$4", mr: "-$4"}} />
</View>

The issue is that the positiveOrNegative function returns "-16" (string) instead of -16. Returning a string for margin is invalid in react native. So on web (chrome) this results in an empty style for the margin, but on iOS in react native this will produce an error something like: json value __ of type nsstring cannot be converted to ygvalue...

See here for a fix https://github.com/levic92/dripsy/commit/1ed71f20abc8ffeac48c3367d1ebc2edc56ed2c7

I wanted to comment on this here because it is somewhat related to the original issue but I do not know if there would be any side effects for just returning a negative number instead of a string.

My previous comment completely demonstrates the bug that this current PR fixes. Please merge it in when you have time, and if you'd like me to submit a PR with the fix for the issue in this comment let me know.

[nandorojo]

Looks like I mixed up the dripsy and moti example packages. In my theme, I create a key for each negative scale:

const space = {
  '-$1': -4
}

That's interesting though. Seems like a reasonable PR.

[nandorojo]

@nandorojo honestly I'm not sure what you're looking for. There is no with-expo folder that I can see. There is an example folder. I will just give you some code here to demonstrate the bug and hopefully you can merge it to fix the behaviour.

example spacing:

space: {
  "4": 16,
  "5": 20,
  "6": 24,
  "7": 28,
  "8": 32,
}

If you have a these 3 views you'd expect the layout to be the same:

<View sx={{p: 8}}>
  <View style={{backgroundColor: "red", width: "100%", height: 100, marginHorizontal: -16}} />
  <View sx={{backgroundColor: "red", width: "100%", height: 100, mx: -4}} />
  <View sx={{backgroundColor: "red", width: "100%", height: 100, ml: -4, mr: -4}} />
</View>

At the moment the top and bottom view would be the same but the middle one will interpret that as -4 instead of -16

I have also found another bug (related to this same concept) I will leave a second comment below

Theme values should be prefixed with $ to avoid this issue. I also recommend using -$ prefixes for negative values.

[levic92]

@nandorojo are you going to merge this in? I’m confused. I think I’ve clearly demonstrated the bug here. It was a mistake in the code to include marginX and marginY in that list. It should have been marginHorizontal and marginVertical from the beginning because the aliases will convert it to that (mx and marginX become marginHorizonatal).

[nandorojo]

I see, so by the time it reaches the transforms, it's already turned into Vertical/Horizontal.