Skip to content

Conversation

landonreed
Copy link
Contributor

@landonreed landonreed commented Nov 12, 2020

Checklist

  • Appropriate branch selected (all PRs must first be merged to dev before they can be merged to master)
  • Any modified or new methods or classes have helpful JSDoc and code is thoroughly commented
  • The description lists all applicable issues this PR seeks to resolve
  • The description lists any configuration setting(s) that differ from the default settings
  • All tests and CI builds passing

Description

Adds form elements to handle setting feed fetch frequency. re ibi-group/datatools-server#346

image

@landonreed landonreed added BLOCKED Blocked (waiting on another PR to be merged) and removed BLOCKED Blocked (waiting on another PR to be merged) labels Nov 13, 2020
Copy link
Contributor

@evansiroky evansiroky left a 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.

@evansiroky evansiroky assigned landonreed and unassigned evansiroky Nov 13, 2020
Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup left a 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.)

Comment on lines +23 to +27
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]
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@evansiroky evansiroky left a 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

@evansiroky evansiroky removed their assignment Nov 16, 2020
@landonreed
Copy link
Contributor Author

Thanks for the thorough review! Comments addressed in 30c72de.

Copy link
Contributor

@binh-dam-ibigroup binh-dam-ibigroup left a 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
Copy link
Contributor

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.

@landonreed landonreed merged commit 6e2a4f5 into dev Nov 17, 2020
@landonreed landonreed deleted the auto-fetch-freq branch November 17, 2020 21:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants