-
Notifications
You must be signed in to change notification settings - Fork 89
Add feed fetch frequency form elements #625
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
Conversation
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.
Some comments about adhering to DRY.
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.
Please see the code cleanup items I mentioned and update the snapshots. (I don't feel strongly about moving the temporal strings to i18n
.)
let {fetchFrequency, fetchInterval} = props | ||
// If frequency and interval are not defined, use default of one fetch per day. | ||
if (!fetchFrequency) fetchFrequency = 'DAYS' | ||
const intervals = FREQUENCY_INTERVALS[fetchFrequency] | ||
if (!fetchInterval) fetchInterval = intervals[0] |
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.
Could be shortened to:
let {fetchFrequency = 'DAYS', fetchInterval = intervals[0]} = props
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.
What if they're null though? Is that possibility precluded with flow typing? I chose this approach because (if I'm not mistaken) if a null
value is passed it will override the default value.
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.
Build is failing, otherwise seems fine
Thanks for the thorough review! Comments addressed in 30c72de. |
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.
Conditional approval on making tests pass.
<span>{messages('fetchFeedEvery')}</span> | ||
{messages('fetchFeedEvery')} | ||
{' '} | ||
<DropdownButton |
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.
Using two dropdowns is probably fine for now, although a single dropdown will all options would ultimately work better.
I can imagine a user trying to adjust the frequency from say 1 day to 12 hours. Intuitively they would try to select the interval first, then adjust the frequency unit. Upon selecting 'hours' their number selection would be reset.
Checklist
dev
before they can be merged tomaster
)Description
Adds form elements to handle setting feed fetch frequency. re ibi-group/datatools-server#346