-
Notifications
You must be signed in to change notification settings - Fork 16
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
2808: Add a filter by date option to events #2898
base: main
Are you sure you want to change the base?
Conversation
…ts by from and to date
…hanging color when opening calendar
…lendarRangeModal + CustomDatePicker for native
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thank you for your PR, looks pretty nice already! Nice work 🎉
I just reviewed and tested native so far and everything works and looks as expected. I will review web
once the comments have been addressed (I think the PR is quite huge, perhaps it would be better to merge it one by one and split up the PR, if possible).
I have a few remarks regarding coding style and complexity. If you feel like I misunderstood something or your solution makes more sense, please tell.
General changes that would be a good addition:
- Use DateTime to avoid manual parsing and formatting as well as validating
- Clicking next to the date picker could/should close it for better UX
- If you delete a slash in the date picker, you can't readd it (since the keyboard only shows numbers). A better choice would perhaps be to implement fields for day, month, and year separately, so you can only edit either one at a time. Not sure if this is an optimal solution though, so if you have any other ideas let me know.
Keep in mind that I did not mention every occurrence of things that could be improved, please apply the suggestions to other parts in the code, too.
release-notes/unreleased/2808-Add-a-filter-by-date-option-to-events.yml
Outdated
Show resolved
Hide resolved
Some thoughts on the behavior:
One more general question: |
Sadly I just realized we have another problem. A lot of our events are recurring, i.e. happening multiple times, for example every week. Our filtering should reflect that and include events that have recurrences in the selected time period. This might not be that easy, so perhaps we should do that in a separate PR. Has to be done before this is good to go though sadly :/ |
I think it can be done by modifying the filter maybe using rrule.between() event.date has |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work, we're getting there! I have a few more general comments:
- As you already noticed, you haven't dealt with the animation yet, and I think it would also be nice to have one in the web.
- While I agree that we should show recurring events in the filtered list, we definitely need to show them with the date that is within that time frame, or show in some other way why these events are shown even though the filter should be hiding them.
- In native, I still find the interactions in the CalendarRangeModal confusing. My expectations from the way other date pickers work would be that the calendar closes when I chose an end date. There are a few other behaviors that I don't expect but I think they would all be covered by closing the calendar when the second date is selected.
- I like your solution for dealing with someone opening an input field a second time, very nice! But I think we still need a better solution for people putting in invalid formats. I think we should let them enter what they want and validate on blur. I would expect a number that is too high to be turned into the maximum possible number, and otherwise invalid dates to either be blanked or get a red outline and an explaining text. I guess an error would be ideal but it’s also more work to add an error behavior for something that, realistically speaking, won’t be happening all that much.
setTempStartDate(startDate) | ||
setTempEndDate(endDate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Why are you setting these temporary values as you're closing the modal? Wouldn't they be filled in correctly anyway with the next opening?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Canceling means putting back old values for calendarModal. When you open the modal again it should show what being selected at inputs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But those are props that are handed in when opening the modal. Have you tested if it works without this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes I test it without ... If I selected a date and pressed cancel and opened the calendar again it will show the same date I selected before canceling. so I think these are needed to get the previous value back
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Tested on Android, works as expected
Can you please add release notes to release-notes/unreleased
folder.
Can you please squash the commits?
I already added one : Release note Squashing into one commit? |
…github.com/digitalfabrik/integreat-app into 2808-Add-a-filter-by-date-option-to-events
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this PR and this new feature. I tested this again on web and everything works as expected.
Good work for staying patient and implement all the requested changes 🚀
Next time it would be easier for you and reviewers to split up PRs in multiple smaller ones.
Yes, please |
…nding on dateFilter
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
Change in average Code Health of affected files: -0.00 (9.84 -> 9.84)
- Declining Code Health: 5 findings(s) 🚩
export const getMarkedDates = ( | ||
startDate: DateTime | null, | ||
endDate: DateTime | null, | ||
theme: ThemeType, | ||
currentInput: string, | ||
): Record<string, MarkedDateType> => { | ||
const markedDateStyling = { | ||
color: theme.colors.themeColor, | ||
textColor: theme.colors.textColor, | ||
} | ||
|
||
const markedDates: Record<string, MarkedDateType> = {} | ||
|
||
const cutoffStartDate = DateTime.now().minus({ years: 2 }) | ||
const cutoffEndDate = DateTime.now().plus({ years: 2 }) | ||
|
||
if (!startDate || startDate < cutoffStartDate || (endDate && endDate > cutoffEndDate)) { | ||
return {} | ||
} | ||
|
||
if (currentInput === 'to' && endDate === null) { | ||
markedDates[startDate.toISODate()] = { | ||
selected: true, | ||
startingDay: false, | ||
endingDay: true, | ||
...markedDateStyling, | ||
} | ||
} else if (currentInput === 'from' && endDate === null) { | ||
markedDates[startDate.toISODate()] = { | ||
selected: true, | ||
startingDay: true, | ||
endingDay: false, | ||
...markedDateStyling, | ||
} | ||
} else { | ||
let runningDate = startDate | ||
const safeEndDate = endDate ?? startDate | ||
while (runningDate <= safeEndDate) { | ||
markedDates[runningDate.toISODate()] = { | ||
selected: true, | ||
startingDay: startDate.equals(runningDate), | ||
endingDay: safeEndDate.equals(runningDate), | ||
...markedDateStyling, | ||
} | ||
runningDate = runningDate.plus({ days: 1 }) | ||
} | ||
} | ||
|
||
return markedDates | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Complex Method
getMarkedDates has a cyclomatic complexity of 10, threshold = 9
native/src/components/DatePicker.tsx
Outdated
const DatePicker = ({ title, date, setDate, error, modalOpen, setModalOpen }: DatePickerProps): ReactElement => { | ||
const { t } = useTranslation('events') | ||
const [inputDay, setInputDay] = useState(date?.toFormat('dd')) | ||
const [inputMonth, setInputMonth] = useState(date?.toFormat('MM')) | ||
const [inputYear, setInputYear] = useState(date?.toFormat('yyyy')) | ||
const [datePickerError, setDatePickerError] = useState('') | ||
const dayRef = useRef<TextInput>(null) | ||
const monthRef = useRef<TextInput>(null) | ||
const yearRef = useRef<TextInput>(null) | ||
|
||
useEffect(() => { | ||
try { | ||
setDatePickerError('') | ||
setDate(DateTime.fromISO(`${inputYear}-${inputMonth}-${inputDay}`)) | ||
} catch (e) { | ||
// This will detect out of range for days | ||
if (!String(e).includes('ISO')) { | ||
setDatePickerError(String(e).replace('Error: ', '')) | ||
} | ||
} | ||
}, [inputDay, inputMonth, inputYear, setDate]) | ||
|
||
useEffect(() => { | ||
if (date) { | ||
setInputDay(date.toFormat('dd')) | ||
setInputMonth(date.toFormat('MM')) | ||
setInputYear(date.toFormat('yyyy')) | ||
} else { | ||
setInputDay('') | ||
setInputMonth('') | ||
setInputYear('') | ||
} | ||
}, [date]) | ||
|
||
return ( | ||
<DateContainer> | ||
<StyledTitle>{title}</StyledTitle> | ||
<StyledInputWrapper> | ||
<Wrapper> | ||
<DatePickerInput | ||
ref={dayRef} | ||
placeholder={t('dd')} | ||
nextTargetRef={monthRef} | ||
inputValue={inputDay} | ||
setInputValue={setInputDay} | ||
type='day' | ||
/> | ||
<Text>.</Text> | ||
<DatePickerInput | ||
ref={monthRef} | ||
placeholder={t('mm')} | ||
nextTargetRef={yearRef} | ||
prevTargetRef={dayRef} | ||
inputValue={inputMonth} | ||
setInputValue={setInputMonth} | ||
type='month' | ||
/> | ||
<Text>.</Text> | ||
<DatePickerInput | ||
style={{ marginLeft: 6 }} | ||
ref={yearRef} | ||
prevTargetRef={monthRef} | ||
placeholder={t('yyyy')} | ||
inputValue={inputYear} | ||
setInputValue={setInputYear} | ||
type='year' | ||
/> | ||
</Wrapper> | ||
<StyledIconButton | ||
$isModalOpen={modalOpen} | ||
icon={<Icon Icon={CalendarTodayIcon} />} | ||
accessibilityLabel={t('common:openCalendar')} | ||
onPress={() => { | ||
setModalOpen(true) | ||
}} | ||
/> | ||
</StyledInputWrapper> | ||
<View style={{ width: '80%' }}> | ||
{!!(error || datePickerError) && <StyledError>{error || datePickerError}</StyledError>} | ||
</View> | ||
</DateContainer> | ||
) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Complex Method
DatePicker has a cyclomatic complexity of 10, threshold = 10
it('should render the given props correctly', () => { | ||
const { getByText } = renderCustomDatePicker({ | ||
modalOpen: false, | ||
setModalOpen, | ||
setDate, | ||
title: 'Test DatePicker', | ||
date: DateTime.now(), | ||
error: '', | ||
}) | ||
|
||
expect(getByText('Test DatePicker')).toBeTruthy() | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Code Duplication
The module contains 5 functions with similar structure: 'should format the day with leading zero on blur','should not allow day greater than 31','should not allow month greater than 12','should render the given props correctly' and 1 more functions
it('should return true when event is within the date range', () => { | ||
const event = createEvent( | ||
1, | ||
now.minus({ days: 1 }), // 2024-11-06 | ||
now.plus({ days: 1 }), // 2024-11-08 | ||
) | ||
const startDate = now.minus({ days: 2 }) // 2024-11-05 | ||
const endDate = now.plus({ days: 2 }) // 2024-11-09 | ||
|
||
const result = isEventWithinRange(event, startDate, endDate) | ||
expect(result).toBe(true) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❌ New issue: Code Duplication
The module contains 5 functions with similar structure: 'should return false when event is outside the date range','should return true when event ends exactly on the endDate','should return true when event has future recurrences within the date range','should return true when event is within the date range' and 1 more functions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
Change in average Code Health of affected files: -0.00 (9.84 -> 9.84)
- Declining Code Health: 5 findings(s) 🚩
…rameters plus refactoring zeroPad
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
Change in average Code Health of affected files: +0.33 (9.20 -> 9.53)
-
Declining Code Health: 5 findings(s) 🚩
-
Affected Hotspots: 1 files(s) 🔥
@@ -570,4 +570,101 @@ describe('DateModel', () => { | |||
]) | |||
}) | |||
}) | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ Getting worse: Code Duplication
introduced similar code in: 'should return recurrences starting from filterStartDate','should return recurrences up to filterEndDate','should return recurrences within the filter date range'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
✅ Code Health Quality Gates: OK
Change in average Code Health of affected files: +0.33 (9.20 -> 9.53)
-
Declining Code Health: 5 findings(s) 🚩
-
Affected Hotspots: 1 files(s) 🔥
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks very good already, thank you for slogging through!
Since we would like to get this live before the winter break, I went with the less pedagogic approach of telling you what exactly to correct instead of trying to lead you there, sorry!
try { | ||
setDate(DateTime.fromISO(event.target.value)) | ||
} catch (_) { | ||
// Invalid date format, do not update the state |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, one more thing that I forgot to mention earlier, sorry: the date in web is still shown as 01/01/1990 instead of 01.01.1990 |
Short description
Filtering events by date
Proposed changes
For web:
CustomDatePicker
and usedInput type='date'
as date picker.For native:
CustomDatePicker
used react-native TextInput and IconButton to show calendar modal.Shared:
Side effects
None.
Resolved issues
Fixes: #2808