2808: Add a filter by date option to events #2898
Conversation
…ts by from and to date
…hanging color when opening calendar
…lendarRangeModal + CustomDatePicker for native
There was a problem hiding this comment.
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 |
|
I just had a short call with Svenja to show her the current state and she suggested also zero-padding the dates here: |
|
When this is merged, we should make a note in this thread that it will be deployed to production the Monday after the next |
There was a problem hiding this comment.
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.
|
|
||
| const textButtonStyles = { | ||
| container: { | ||
| height: 40, |
There was a problem hiding this comment.
❌ The height can go directly into the StyledTextButton
There was a problem hiding this comment.
Yeah I tried that.. its common sense to put it in StyledTextButton but that didn't work..😅
There was a problem hiding this comment.
That's weird, it worked when I tested it, only the font size needed to be added in-line 🤔
| setTempStartDate(startDate) | ||
| setTempEndDate(endDate) |
There was a problem hiding this comment.
❓ 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.
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.
But those are props that are handed in when opening the modal. Have you tested if it works without this?
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.
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
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
| 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.
❌ New issue: Complex Method
getMarkedDates has a cyclomatic complexity of 10, threshold = 9
| const cutoffStartDate = DateTime.now().minus({ years: 2 }) | ||
| const cutoffEndDate = DateTime.now().plus({ years: 2 }) | ||
|
|
||
| if (!startDate || startDate < cutoffStartDate || (endDate && endDate > cutoffEndDate)) { |
There was a problem hiding this comment.
❌ New issue: Complex Conditional
getMarkedDates has 1 complex conditionals with 3 branches, threshold = 2
| 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.
❌ 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.
❌ 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.
❌ 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
…rameters plus refactoring zeroPad
| @@ -570,4 +570,101 @@ describe('DateModel', () => { | |||
| ]) | |||
| }) | |||
| }) | |||
|
|
|||
There was a problem hiding this comment.
ℹ 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'
Short description
Filtering events by date
Proposed changes
For web:
CustomDatePickerand usedInput type='date'as date picker.For native:
CustomDatePickerused react-native TextInput and IconButton to show calendar modal.Shared:
Side effects
None.
Resolved issues
Fixes: #2808