-
Notifications
You must be signed in to change notification settings - Fork 73
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
LF-3637(3): Update the date inputs used in date range selector [integration] #2913
Changes from all commits
223ad3c
567a3d8
51c9524
1870284
33724ce
152603e
21419ac
780ddf6
878974a
df08a52
5cf65ec
3495ba3
d680a10
19186b5
a551c95
c8e98b8
7858554
d2dfb39
e0deb88
65883bd
c693422
bd78b95
8c370c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
/* | ||
* Copyright 2023 LiteFarm.org | ||
* This file is part of LiteFarm. | ||
* | ||
* LiteFarm is free software: you can redistribute it and/or modify | ||
* it under the terms of the GNU General Public License as published by | ||
* the Free Software Foundation, either version 3 of the License, or | ||
* (at your option) any later version. | ||
* | ||
* LiteFarm is distributed in the hope that it will be useful, | ||
* but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
* GNU General Public License for more details, see <https://www.gnu.org/licenses/>. | ||
*/ | ||
import { useEffect } from 'react'; | ||
import { useDispatch, useSelector } from 'react-redux'; | ||
import PropTypes from 'prop-types'; | ||
import { dateRangeDataSelector } from '../../containers/Finances/selectors'; | ||
import DateRange, { MONDAY, SUNDAY } from '../../util/dateRange'; | ||
import { dateRangeOptions } from './constants'; | ||
import { setDateRange } from '../../containers/Finances/actions'; | ||
|
||
/** | ||
* Returns startDate and endDate in the store. | ||
* If they are not defined, set the default option and dates. | ||
*/ | ||
export default function useDateRangeSelector({ weekStartDate = SUNDAY }) { | ||
const dateRange = useSelector(dateRangeDataSelector); | ||
const dispatch = useDispatch(); | ||
|
||
const dateRangeUtil = new DateRange(new Date(), weekStartDate); | ||
const option = dateRange.option || dateRangeOptions.YEAR_TO_DATE; | ||
|
||
useEffect(() => { | ||
if ((!dateRange.startDate || !dateRange.endDate) && option !== dateRangeOptions.CUSTOM) { | ||
const { startDate, endDate } = dateRangeUtil.getDates(option); | ||
dispatch(setDateRange({ option, startDate, endDate })); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Couldn't we set this start date and end date as the initial state on the finance reducer? That way we probably wouldn't need this hook, and we would have the default state setup on the first render. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's possible, but we might want to change "start date" of the week (Sunday or Monday) depending on the user in the future, so I decided not to set the default in the reducer. |
||
} | ||
}, []); | ||
|
||
return { startDate: dateRange.startDate, endDate: dateRange.endDate }; | ||
} | ||
|
||
useDateRangeSelector.propTypes = { | ||
weekStartDate: PropTypes.oneOf([SUNDAY, MONDAY]), | ||
}; |
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.
Small thing I didn't notice before -- there is a slight but handled naming conflict with dateRangeSelector from the state selectors in Finances.
/components/Finances/DateRangeSelector/index.jsx
is at least one file that imports both {dateRangeSelector} and DateRangeSelector.I don't have any great name alternatives haha
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.
Is it known to cause any problems??
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.
It's just a best practice to avoid having the same name in one file. The main problem with non unique names is developer confusion (in javascript). As you know this will work. Communication becomes harder.
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.
Understood.