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

Address Guidehouse feedback on landing page #450

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

brookslogan
Copy link
Contributor

Checklist

Please:

  • Make sure this PR is against "dev", not "main" (unless this is a release
    PR).
  • Request a review from one of the current main reviewers:
    brookslogan, nmdefries.
  • Makes sure to bump the version number in DESCRIPTION. Always increment
    the patch version number (the third number), unless you are making a
    release PR from dev to main, in which case increment the minor version
    number (the second number).
  • Describe changes made in NEWS.md, making sure breaking changes
    (backwards-incompatible changes to the documented interface) are noted.
    Collect the changes under the next release number (e.g. if you are on
    1.7.2, then write your changes under the 1.8 heading).
  • See DEVELOPMENT.md for more information on the development
    process.

Change explanations for reviewer

This addresses some feedback we received from Guidehouse on the landing page; here are my notes:

  • We should provide more context on landing page: why, what need, what ?? addresses; highlighting more e.g. the top 2 novel features.
  • Useful in landing page to have a little more detail e.g. about 7dav-ing?
  • Add to landing page the expected level we expect people to have to use the package (we expect people to be able to use these packages in R that are very common e.g., dplyr and tidyr).

There is some slightly-awkward link spam for {epiprocess} and {epipredict} so that we retain braces around all package names; if we don't manually turn them into links, then they will automatically be turned into links anyway and have the braces dropped (and a slightly worse link for epipredict, probably because we list the repo URL before the github.io URL in the DESCRIPTION file).

Magic GitHub syntax to mark associated Issue(s) as resolved when this is merged into the default branch

@brookslogan brookslogan requested a review from nmdefries May 28, 2024 21:52
@brookslogan brookslogan force-pushed the lcb/address-guidehouse-feedback branch 2 times, most recently from b75387d to de293f0 Compare May 29, 2024 17:34
@brookslogan brookslogan force-pushed the lcb/address-guidehouse-feedback branch from de293f0 to d3827a3 Compare May 29, 2024 17:39
@brookslogan
Copy link
Contributor Author

Rebased & force-pushed to resolve DESCRIPTION + NEWS.md conflicts, plus improved a little wording in NEWS.md.

Copy link
Contributor

@nmdefries nmdefries left a comment

Choose a reason for hiding this comment

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

Looks good. Suggestions are optional.

This package introduces a common data structure for epidemiological data sets
measured over space and time, and offers associated utilities to perform basic
signal processing tasks.
The [`{epiprocess}`](https://cmu-delphi.github.io/epiprocess/) package provides
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion: give broad intro before detailed list

Suggested change
The [`{epiprocess}`](https://cmu-delphi.github.io/epiprocess/) package provides
The [`{epiprocess}`](https://cmu-delphi.github.io/epiprocess/) package introduces a common data structure for epidemiological data sets
measured over space and time, and offers associated utilities to perform basic
signal processing tasks.
The package provides the following tools:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That intro intro's not accurate, I've taken a shot at an accurate one.

vignettes/epiprocess.Rmd Outdated Show resolved Hide resolved
@brookslogan brookslogan merged commit 0ce39c5 into dev Jun 4, 2024
4 checks passed
@dshemetov dshemetov deleted the lcb/address-guidehouse-feedback branch June 4, 2024 22:45
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.

2 participants