-
-
Notifications
You must be signed in to change notification settings - Fork 23
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 blog post for conda-based installers #214
Conversation
8190918
to
ca0ad93
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per our discussion, I went ahead and made commits to add you as an author and resolve a couple other technical issues, fix the line breaks, and downscale and optimize the images. This makes them display at a much more appropriate and non-pixelated size (as they appear upscaled from their originals, perhaps due to hiDPI), and reduces their file size by between 4x and 10x, for relatively minimal visible quality loss.
Additionally, I made non-trivial comments on the text as suggestions, which as always you can apply by going to Files
-> Add to batch [for each] -> Commit.
Thanks!
Co-authored-by: C.A.M. Gerlach <[email protected]>
@CAM-Gerlach, all your suggestions looked good to me; thank you for reviewing! |
Thanks @mrclary ! Looks pretty good to me now, but for some reason the checks didn't fire as they should have—I'll close and re-open to see if that will proc them, and assuming everything's good I'll ping @ccordoba12 for a final review. Thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything now LGTM from me; thanks @mrclary ! I'll leave this to @ccordoba12 to give this a final review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mrclary and @CAM-Gerlach for your work on this!
Thanks for responses, @ccordoba12. I think they were very helpful. I had just a few counter-suggestions; when we reach consensus, I'll commit all suggestions together. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My further recommended revisions, where applicable, of @ccordoba12 's suggested changes.
EDIT: Note that @mrclary 's comments didn't show up for me when I was reviewing (since I started the review last night and only finished now), so I'm going back and addressing them.
Co-authored-by: C.A.M. Gerlach <[email protected]> Co-authored-by: Carlos Cordoba <[email protected]>
@ccordoba12 and @CAM-Gerlach, seems like we have converged. Thanks for your feedback. Whoever merges this, remember to update the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me now, thanks @mrclary!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now as well, thanks @mrclary !
Co-authored-by: C.A.M. Gerlach <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your patience @mrclary. This is finally going in!
No description provided.