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

Fix DateInlineRow, TimeInlineRow and DateTimeInlineRow style/rend… #2067

Merged
merged 6 commits into from
Sep 15, 2020
Merged

Conversation

kamerc
Copy link
Contributor

@kamerc kamerc commented Aug 25, 2020

Fix DateInlineRow, TimeInlineRow and DateTimeInlineRow style/rendering issues for iOS14. See screenshots from the Example app for details. The default picker view is .inline to match the iOS native apps. If a user is on iOS 13 or lower, they will still see the default style of wheels

DateInlineRow (before)

dateBefore

DateInlineRow (after)

dateAfter

TimeInlineRow (before)

timeBefore

TimeInlineRow (after)

timeAfter

DateTimeInlineRow (before)

dateTimeBefore

DateTimeInlineRow (after)

dateTimeAfter

@funkenstrahlen
Copy link

Thank you!

onExpandInlineRow { cell, row, _ in
onExpandInlineRow { cell, row, inlineRow in
inlineRow.cellUpdate() { cell, row in
cell.datePicker.datePickerMode = .date
Copy link
Member

Choose a reason for hiding this comment

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

I think setting datePickerMode is not needed here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

From the documentation: default is UIDatePickerModeDateAndTime. So this line needs to stay for DateInlineRow and also the line that sets it to .time on the TimeInlineRow. But it looks like the line for DateTimeInlineRow that sets it to .dateAndTime can be removed.

Copy link
Member

Choose a reason for hiding this comment

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

I mean datePickerMode is set in DatePickerCell (line 54). It is not necessary to re-set it here

@mats-claassen
Copy link
Member

I agree with the change but I think onExpandInlineRow isn't the best place for this as anyone who overrides this has to copy this code as well to maintain the functionality. From what I saw, if we remove the line setting the datePickerMode it is the same code for each date row except CountDownRow which does not support .inline mode. What about adding this change to configureInlineRow in DatePickerRowProtocol? You will have to check whether datePickerMode is countdown to avoid applying this change to CountDown rows

@kamerc
Copy link
Contributor Author

kamerc commented Aug 27, 2020

@mats-claassen configureInlineRow takes DatePickerRowProtocol rather than DatePickerRow, which I need to be able to access the cell.datePicker. What if I add a new method to call from setupInlineRow that passes DatePickerRow?

@mats-claassen
Copy link
Member

Sounds good. Then just don't call it in CountDown row

kamerc added 2 commits August 27, 2020 12:27
…ering issues for iOS14.

Update with changes from code review
…ering issues for iOS14.

Revert onExpandInlineRow
@kamerc
Copy link
Contributor Author

kamerc commented Aug 27, 2020

@mats-claassen I have pushed an update

kamerc and others added 2 commits August 27, 2020 20:06
…ering issues for iOS14.

Change parameter on configurePickerStyle to DatePickerCell since the function is only changing the cell on the datePickerRow
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.

3 participants