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 query variables for easy linking #291

Merged
merged 11 commits into from Oct 7, 2020

Conversation

cameronmarlow
Copy link
Contributor

It's currently hard to share microcovid results for a specific set of assumptions. It would be really nice to, say, link to the assumptions for the presidential debate. This PR adds the use-query-param library and automatically updates query params based on changes to the form for easy sharing.

  • Add user-query-params library, upgrade query-string
  • Create a configuration for query parameters
  • Update query parameters whenever calculator changes
  • Adjust scroll behavior to avoid changing variables

* Add user-query-params library, upgrade query-string
* Create a configuration for query parameters
* Update query parameters whenever calculator changes
* Adjust scroll behavior to avoid changing variables
@netlify
Copy link

netlify bot commented Oct 4, 2020

Deploy preview for microcov ready!

Built with commit 9d079cd

https://deploy-preview-291--microcov.netlify.app

@blanchardjeremy
Copy link
Member

I don't know how to fix the remaining 3 lint issues.

* Fix return types
@cameronmarlow
Copy link
Contributor Author

Wow, thanks! I was planning to get back to lint today

@cameronmarlow cameronmarlow changed the title [WIP] Add query variables for easy linking Add query variables for easy linking Oct 4, 2020
Copy link
Member

@blanchardjeremy blanchardjeremy left a comment

Choose a reason for hiding this comment

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

image

I'm getting this error when I try to run your PR locally. And the netlify version isn't working either: https://deploy-preview-291--microcov.netlify.app

Can you take a look into this and let us know what you see?

@cameronmarlow
Copy link
Contributor Author

Strange, I see this now too.

By default, the query object contains undefined values.

I'm not sure if this is exactly the right API, but it shoudl be working in the base case now.
Copy link
Member

@beshaya beshaya left a comment

Choose a reason for hiding this comment

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

This is a great idea; I really like that it allows sharing and linking!

src/data/queryParams.ts Outdated Show resolved Hide resolved
src/pages/Calculator.tsx Outdated Show resolved Hide resolved
src/data/queryParams.ts Outdated Show resolved Hide resolved
src/data/queryParams.ts Outdated Show resolved Hide resolved
* use pickBy for filterParams
* More descriptive funcion name for useQueryData, better documentation
* Move useQueryParams call to Calculate
@blanchardjeremy
Copy link
Member

Question/idea:

I wonder if it would be a better user experience to not automatically this long series of params to the URL and instead to give them a "send a link to this scenario to a friend" button that automatically copies it to the clipboard. Thoughts?

@cameronmarlow
Copy link
Contributor Author

@blanchardjeremy I had the same thought - If you wanted to install a block of share buttons it might make more sense to move this functionality there. This is slightly more aggressive from a growth perspective but it's really a cosmetic choice in the end.

@cameronmarlow
Copy link
Contributor Author

Also I'm not sure how to re-run CircleCI builds, but the lint errors seem to be unrelated to the code

@blanchardjeremy blanchardjeremy linked an issue Oct 6, 2020 that may be closed by this pull request
@blanchardjeremy
Copy link
Member

@cameronmarlow hmmm. I haven't tried to run the code yet. You've probably already done this, but did you run "yarn fix")

@blanchardjeremy
Copy link
Member

I'm seeing this erroe

[1/4] Resolving packages...
14
error Your lockfile needs to be updated, but yarn was run with --frozen-lockfile.
15
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.

I'm not 100% sure what the source of that is either. Maybe delete your package log and run yarn install?

@blanchardjeremy
Copy link
Member

@cameronmarlow we are hoping to get this ready to go in the next day or two for a reporter who wants to link to specific scenarios.

So let me know if I can support in any way!

@blanchardjeremy
Copy link
Member

@cameronmarlow I propose we merge this pull request and then start the work on migrating it to a button (or series of share buttons) next. Would you be open to heading up that effort?

@blanchardjeremy
Copy link
Member

image

Hmmm. Well, the netlify preview is now working after I ran yarn install. But I'm not getting it working locally! Is this just me or are others seeing this?

@beshaya
Copy link
Member

beshaya commented Oct 7, 2020

Looks good to me now. Thanks so much for this, @cameronmarlow!

@beshaya beshaya merged commit 830a5bb into microCOVID:main Oct 7, 2020
@cameronmarlow
Copy link
Contributor Author

Thanks for getting this over the finish line! I'll take a pass at translating this into a button tomorrow.

@cameronmarlow cameronmarlow deleted the feature/add-query-variables branch October 7, 2020 04:38
@blanchardjeremy
Copy link
Member

@cameronmarlow your feature made it possible for this news article to link to specific scenarios: https://www.vox.com/21504747/trump-coronavirus-superspreader

Thanks! 🙌

@cameronmarlow
Copy link
Contributor Author

Awesome! That's exactly why I got interested in it. I was trying to do the same thing myself:

https://overstated.net/2020/10/02/calculating-risk-with-microcovids/

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.

No ability to share custom scenarios?
3 participants