Skip to content
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

Add API key field to covidcast imports #55

Open
wants to merge 9 commits into
base: dev
Choose a base branch
from
Open

Conversation

rzats
Copy link
Contributor

@rzats rzats commented Aug 12, 2024

Closes #47:

  • Adds an API key field to the COVIDCast signal import dialog.
  • This displays previously hidden private signals in the dropdown, making them available for visualization.

@rzats rzats requested a review from melange396 August 16, 2024 12:49
@rzats
Copy link
Contributor Author

rzats commented Aug 16, 2024

@melange396 also fixed!

Copy link
Contributor

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works, but it doesnt do its verification until the focus is moved off of the input field, and it doesnt repopulate the metadata until verified... The delay can be confusing and will be frustrating, unless perhaps we include a "working" indicator or similar. Did you explore triggering the key verification on typing (the on:input event instead of on:change)?

src/components/dialogs/dataSources/COVIDcast.svelte Outdated Show resolved Hide resolved
src/components/dialogs/dataSources/COVIDcast.svelte Outdated Show resolved Hide resolved
src/components/dialogs/dataSources/COVIDcast.svelte Outdated Show resolved Hide resolved
src/components/dialogs/dataSources/COVIDcast.svelte Outdated Show resolved Hide resolved
src/components/dialogs/dataSources/COVIDcast.svelte Outdated Show resolved Hide resolved
@rzats
Copy link
Contributor Author

rzats commented Sep 24, 2024

@melange396

This works, but it doesnt do its verification until the focus is moved off of the input field, and it doesnt repopulate the metadata until verified... The delay can be confusing and will be frustrating, unless perhaps we include a "working" indicator or similar. Did you explore triggering the key verification on typing (the on:input event instead of on:change)?

I moved the key verification to on:input with a debounce that makes sure it only activates once the user stopped typing for 500ms. The timer can be adjusted, try it out and see what you think!

Copy link
Contributor

@melange396 melange396 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment. Otherwise, the oninput w/ delayed request looks great!

src/components/dialogs/dataSources/COVIDcast.svelte Outdated Show resolved Hide resolved
@rzats rzats requested a review from melange396 October 1, 2024 13:48
@@ -357,6 +369,7 @@ export function importFluView({
'num_age_4',
'num_age_5',
],
'',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ick! The need to include this empty string is an ugly/unfortunate side effect from the recent addition of the columnRenamings argument to loadDataSet() in #66 (and because TypeScript doesnt have a "proper" named optional argument syntax).

Does this work? Its feels a lot less hacky:

Suggested change
'',
auth, # also remove `auth` from the `userParams` arg a few lines above

We should standardize all of the other "importFoo()" methods (for the endpoints that use an "authorization token") so they use the api_key argument of loadDataSet() too. I can see some rationale for doing that in PR #72, but its big enough already, and it doesnt touch this file, and the argument was introduced in this PR, so i think it belongs here.

@rzats rzats requested a review from melange396 October 10, 2024 14:05
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.

Add "API Key" field to COVIDcast imports
2 participants