Skip to content

Custom scroll in year picker and allow editing button #202

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Mar 4, 2023
Merged

Custom scroll in year picker and allow editing button #202

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8122aea
feat: add custom scroll and allow editing button
HeartiLab Sep 13, 2022
b88011f
Merge pull request #1 from TVGSoft/feature/feat-year-calendar-indepen…
TVGSOFT Sep 13, 2022
5764a2e
Merge branch 'master' into master
TVGSOFT Mar 3, 2023
09c1ffc
Update YearPicker.tsx
TVGSOFT Mar 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/Date/DatePickerModalContent.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,7 @@ export function DatePickerModalContent(
locale={locale}
editIcon={props?.editIcon}
calendarIcon={props.calendarIcon}
allowEditing={props.allowEditing || true}
Copy link
Contributor

@jcloquell jcloquell Mar 7, 2023

Choose a reason for hiding this comment

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

This is always true. Shouldn't it be {props.allowEditing ?? true} instead?

Copy link
Contributor Author

@TVGSOFT TVGSOFT Mar 8, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @jcloquell Yup I agree.
it should be allowEditing={props.allowEditing ?? true}

/>
</DatePickerModalHeaderBackground>
<AnimatedCrossView
Expand Down
14 changes: 7 additions & 7 deletions src/Date/DatePickerModalContentHeader.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ export interface HeaderPickProps {
editIcon?: string
calendarIcon?: string
closeIcon?: string
allowEditing?: boolean
}

export interface HeaderContentProps extends HeaderPickProps {
Expand Down Expand Up @@ -62,19 +63,18 @@ export default function DatePickerModalContentHeader(
editIcon,
calendarIcon,
inputDate,
allowEditing,
} = props
const theme = useTheme()
const label = getLabel(props.locale, props.mode, props.label)

const color = useHeaderTextColor()
const supportingTextColor = theme.isV3 ? theme.colors.onSurfaceVariant : color
let allowEditing = mode !== 'multiple'

if (inputDate !== undefined) {
allowEditing = inputDate
}
const isAllowEditing = allowEditing && mode !== 'multiple' && inputDate !== undefined
Copy link
Contributor

@jcloquell jcloquell Mar 7, 2023

Choose a reason for hiding this comment

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

I think this change brought some undesired side effects, as

if (inputDate !== undefined) {
    allowEditing = inputDate
}

is not equivalent to making const isAllowEditing = ... && inputDate !== undefined.

The first is overriding the value of isAllowEditing with inputDate if this is defined.
The second is having an impact on the isAllowEditing value depending on inputDate being defined or not.

At the same time, the existing inputDate property is and was never set from anywhere outside where this component is used, so its value is always undefined.

All of this make the edit button to always be hidden regardless the value passed for the allowEditing prop.

Copy link
Collaborator

@iM-GeeKy iM-GeeKy Mar 7, 2023

Choose a reason for hiding this comment

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

Hi @jcloquell, thanks for catching this! Would you mind creating a quick PR to address this?

Copy link
Contributor Author

@TVGSOFT TVGSOFT Mar 8, 2023

Choose a reason for hiding this comment

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

@jcloquell i think can change to this:
const isAllowEditing = allowEditing && mode !== 'multiple'

And remove this:
if (inputDate !== undefined) { allowEditing = inputDate }

What do you think?

Copy link
Contributor

@jcloquell jcloquell Mar 8, 2023

Choose a reason for hiding this comment

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

Sure! Here is the PR - #264 :)


const supportingTextColor = theme.isV3 ? theme.colors.onSurfaceVariant : color

let textFont = theme?.isV3
const textFont = theme?.isV3
? theme.fonts.labelMedium
: (theme as any as MD2Theme).fonts.medium

Expand Down Expand Up @@ -103,7 +103,7 @@ export default function DatePickerModalContentHeader(
</View>
</View>
<View style={styles.fill} />
{allowEditing ? (
{isAllowEditing ? (
<IconButton
icon={
collapsed
Expand Down
5 changes: 4 additions & 1 deletion src/Date/YearPicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import * as React from 'react'
import { FlatList, StyleSheet, View } from 'react-native'
import { FlatList, StyleSheet, View, ScrollView } from 'react-native'
import { MD2Theme, Text, TouchableRipple, useTheme } from 'react-native-paper'
import { range } from '../utils'

Expand Down Expand Up @@ -50,6 +50,9 @@ export default function YearPicker({
ref={flatList}
style={styles.list}
data={years}
renderScrollComponent={(sProps) => {
return <ScrollView {...sProps} />
}}
renderItem={({ item }) => (
<Year
year={item}
Expand Down