BugiDev / react-native-calendar-strip

Easy to use and visually stunning calendar component for React Native.
MIT License
936 stars 325 forks source link

Add the property dayComponentHeight to handle day height #273

Closed pedrobslisboa closed 3 years ago

pedrobslisboa commented 3 years ago

What it does:

Before

After

WhatsApp Image 2021-02-10 at 14 42 29

This branch/PR was made based on this discussion #222 and this PR #223

@peacechen Any problem/issue, please let me know :)

peacechen commented 3 years ago

Thanks @pedrobslisboa for updating this PR. Please see the questions above about the size.height prop change check.

pedrobslisboa commented 3 years ago

@peacechen nice questions For instance, we need to comprehend how it can affect this part of the code

https://github.com/BugiDev/react-native-calendar-strip/blob/70e60fb7a53fc25784f666a2ff41b16febdeb3a2/src/CalendarDay.js#L151-L158 https://github.com/BugiDev/react-native-calendar-strip/blob/70e60fb7a53fc25784f666a2ff41b16febdeb3a2/src/CalendarDay.js#L438-L442

I guess we should handle it either with height and width instead of generic 'size'

So we can have:

containerSize: {
  width: Math.round(props.size.width),
  height: Math.round(props.size.height),
} 

Changing, of course, every part which deals with it.

I'm going to test it, but what do you think about it?

peacechen commented 3 years ago

Good catch. Those should also calculate width & height since it's no longer guaranteed to be square. However the borderRadius takes only a single value, so it will need to pick one of the dimensions.

Instead of a size prop in CalendarDay.js, let's pass separate width and height props. Height defaults to width, and most calculations use width by default.

Update: There are now merge conflicts in this PR. Please update to the latest in the main branch.

peacechen commented 3 years ago

It looks like you're using an auto-formatter which introduced a lot of non-functional changes. I'm not completely opposed to it, but I'm concerned that someone else's auto-formatter will come back and revert those non-functional formatting changes. Can you turn that off for these files?

pedrobslisboa commented 3 years ago

You're right, I have a practice to always use a shortcut to format the code, my mistake.

I've opened an issue to handle every issue like this.

https://github.com/BugiDev/react-native-calendar-strip/issues/274

pedrobslisboa commented 3 years ago

@peacechen Merge done and format undone

peacechen commented 3 years ago

Not sure if you saw this comment above

Instead of a size prop in CalendarDay.js, let's pass separate width and height props. Height defaults to width, and most calculations use width by default.

It would be clearer to explicitly pass individual height & width props instead of a size object prop.

pedrobslisboa commented 3 years ago

Not sure if you saw this comment above

Instead of a size prop in CalendarDay.js, let's pass separate width and height props. Height defaults to width, and most calculations use width by default.

It would be clearer to explicitly pass individual height & width props instead of a size object prop.

I didn't see it, sorry. It's fixed already

peacechen commented 3 years ago

Thanks @pedrobslisboa this looks good. How much testing has this code gotten? We need to make sure it's solid before publishing. You can use the sample project in this repo to try different props: https://github.com/BugiDev/react-native-calendar-strip#development-with-sample-application

pedrobslisboa commented 3 years ago

@peacechen I've tested it both on my android and the IOS simulator. But I am going to repeat the tests tomorrow giving you some screenshots. The only one I've made is the one at the description.

pedrobslisboa commented 3 years ago

@peacechen

Landscape without scrollable

https://user-images.githubusercontent.com/35539594/107632721-a44d3a80-6c45-11eb-8459-a1b7d2cc091a.mp4

Portrait without scrollable

https://user-images.githubusercontent.com/35539594/107632729-a57e6780-6c45-11eb-8a47-effdd77db9b1.mp4

Landscape with scrollable

https://user-images.githubusercontent.com/35539594/107632731-a616fe00-6c45-11eb-90a7-f5bd4acaab2d.mp4

peacechen commented 3 years ago

Thanks for thoroughly testing 👍