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.

Sign up for GitHub

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

Jump to bottom

doc+ci: match ci and style with epipredict/epidatr #418

Merged
merged 25 commits into from
Mar 9, 2024
Merged

doc+ci: match ci and style with epipredict/epidatr #418

merged 25 commits into from
Mar 9, 2024

Conversation

dshemetov
Copy link
Contributor

@dshemetov dshemetov commented Feb 16, 2024 •
edited
Loading

Checklist

Please:

  • Make sure this PR is against "dev", not "main".
  • Request a review from one of the current epiprocess main reviewers:
    brookslogan, nmdefries.
  • Makes sure to bump the version number in DESCRIPTION and NEWS.md.
    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
    0.7.2, then write your changes under the 0.8 heading).

Change explanations for reviewer

Match with

Contains #425

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

@dshemetov dshemetov requested a review from dsweber2 March 5, 2024 01:34
@dshemetov
doc+ci+repo: fix style and release guide
9885f70 
* update with fixes from epidatr
* update pull request template
* bugfix pkgdown.yaml
* add workflow_dispatch buttons to most actions
* remove release_helper.yaml which we don't use
* update the DEVELOPMENT.md guide with a release checklist
* update pkgdown.yaml to match epidatr
@dshemetov dshemetov requested a review from dajmcdon March 5, 2024 01:41
@dsweber2
Copy link
Contributor

dsweber2 commented Mar 5, 2024

Request a review from one of the current epiprocess main reviewers:
brookslogan, nmdefries.

Currently tagged:
Dan and David

was that intentional, or is the robot set up to tag the wrong people?

@dsweber2
Copy link
Contributor

dsweber2 commented Mar 5, 2024 •
edited
Loading

I guess its worth noting that the pkgdown is hopefully temporary until this PR is merged. Though version_label = "success" is actively useful to get the version color to not be a red that's nearly indistinguishable from the CMU banner.

@dshemetov dshemetov requested a review from brookslogan March 5, 2024 21:18
@dshemetov
Copy link
Contributor Author

dshemetov commented Mar 5, 2024 •
edited
Loading

Given the amount of outstanding PRs on that repo and last release in 2022, I'm not holding my breath for a quick merge.

There's no robo tagging on this repo and I just tagged you because you reviewed the other PR already.

@dshemetov dshemetov removed the request for review from dajmcdon March 6, 2024 06:50
dsweber2
dsweber2 approved these changes Mar 6, 2024
View reviewed changes
Copy link
Contributor

@dsweber2 dsweber2 left a comment

Choose a reason for hiding this comment

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

Matches what we did in epidatr. Might want to wait to see if others want to comment on the process before merging.

brookslogan
brookslogan approved these changes Mar 7, 2024
View reviewed changes
Copy link
Contributor

@brookslogan brookslogan left a comment

Choose a reason for hiding this comment

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

Very nice. Mostly asking for some extra details in some of the development docs but have some random other comments.

.github/workflows/pkgdown.yaml Outdated Show resolved Hide resolved
.github/workflows/pkgdown.yaml Show resolved Hide resolved
.github/workflows/pkgdown.yaml Outdated Show resolved Hide resolved
.github/workflows/pkgdown.yaml Outdated Show resolved Hide resolved
.github/workflows/pkgdown.yaml Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
DEVELOPMENT.md Outdated Show resolved Hide resolved
_pkgdown.yml Show resolved Hide resolved
.github/pull_request_template.md Show resolved Hide resolved
dshemetov and others added 14 commits March 6, 2024 17:17
@dshemetov @brookslogan
Update .github/workflows/pkgdown.yaml
6b325da 
Co-authored-by: brookslogan <[email protected]>
@dshemetov @brookslogan
Update DEVELOPMENT.md
c16b98b 
Co-authored-by: brookslogan <[email protected]>
@dshemetov
ci: add document, lint, style like epidatr
219e04a
@dshemetov
ci: add doc string with workflow mods
b40f90f
@dshemetov
ci: fix style indentation
cbf4e44
@dshemetov
doc: improve release process wording
4a941d2
@dshemetov
doc: add link to DEVELOPMENT.md in pull request template
aa52363
@dshemetov
ci: simplify pkgdown ref logic
f89ecdd
@dshemetov
doc: transfer maintainer role from Ryan to Logan
a48d82c
@dshemetov
doc: add comments explaining pkgdown action
9143422 
* also simplify the target_ref calculation
@dshemetov
doc: clarify DEVELOPMENT.md
1a0a885
@dshemetov
Merge pull request #425 from cmu-delphi/ds/ci
d6f3675 
ci: add document, lint, style like epidatr
@dshemetov
ci: fix pkgdown
6fd2e1f
@dsweber2
More color-blind friendly green
b19921a
@dshemetov dshemetov changed the title doc: match style with epipredict doc: match style with epipredict/epidatr Mar 7, 2024
@dshemetov dshemetov changed the title doc: match style with epipredict/epidatr doc+ci: match ci and style with epipredict/epidatr Mar 7, 2024
@dshemetov
Copy link
Contributor Author

dshemetov commented Mar 7, 2024 •
edited
Loading

@brookslogan I fixed a lot of linter unhappiness. but the remaining bits I wanted your input on. Check the failed CI above and please let me know what you think. Never mind, just ignored the remaining ones. After the colors are finalized above, I think this is good to go?

dajmcdon
dajmcdon approved these changes Mar 8, 2024
View reviewed changes
Copy link
Contributor

@dajmcdon dajmcdon left a comment

Choose a reason for hiding this comment

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

One other potential color thing. Possibly challenging, but mentioned in cmu-delphi/epipredict#289.

@dsweber2
Copy link
Contributor

dsweber2 commented Mar 8, 2024 •
edited
Loading

we could fix the search bar pretty quickly by switching off of cosmo- was there some reason we chose that?

@dajmcdon
Copy link
Contributor

dajmcdon commented Mar 8, 2024

I don't think so. Probably just a function of me not understanding the details of the bootstrap5 functionality.

@dsweber2
Copy link
Contributor

dsweber2 commented Mar 8, 2024

image
switching the navbar style to light seems to have addressed the search bar at least, and made the box white rather than black (dunno if that's better or worse).

@brookslogan
Copy link
Contributor

I prefer the white search bar over the black one when the background for everything else is white. (Assuming text is visible when you type there.)

@dajmcdon
Copy link
Contributor

dajmcdon commented Mar 8, 2024

Agreed. I like the white one much better.

@dshemetov dshemetov merged commit efab89e into dev Mar 9, 2024
3 checks passed
@dshemetov dshemetov deleted the ds/docs-style branch March 9, 2024 00:04
@nmdefries nmdefries mentioned this pull request Mar 14, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brookslogan brookslogan brookslogan approved these changes

@dsweber2 dsweber2 dsweber2 approved these changes

@dajmcdon dajmcdon dajmcdon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dshemetov @dsweber2 @dajmcdon @brookslogan