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

Adding figure with lesson goal/overview #891

Merged
merged 1 commit into from
Jan 29, 2021

Conversation

vcheplygina
Copy link
Contributor

Instructions

As discussed with @maxim-belkin, added overview figure and clarified the text a bit to add more context to the lesson.

Copy link
Contributor

@ldko ldko left a comment

Choose a reason for hiding this comment

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

I think the image adds a nice and informative overview of the lesson story and the text change makes things a bit clearer. I commented on a couple of minor technical details. What do you think @maxim-belkin ? Thanks very much for the addition, @vcheplygina !

index.md Outdated Show resolved Hide resolved
index.md Outdated Show resolved Hide resolved
@vcheplygina
Copy link
Contributor Author

Thanks, good to hear! I tried to fix both in 7a81324 and 96673a6, I hope this is working as intended

Copy link
Contributor

@ldko ldko left a comment

Choose a reason for hiding this comment

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

Hi @vcheplygina I appreciate your modifying the image per my request. I checked with co-maintainer @maxim-belkin wondering about the pptx format. While a raw file is good to include, he indicated that pptx is not ideal because it is actually a zip archive. If possible, including an svg would be wonderful, and he recommended ImageMagick for that. One other wish list item from him was spacing the vertical alignment on the rows of numbers to more closely align with the patient icons. Let me know what you think about these requests--we are happy to have you working on this! Thank you!

index.md Outdated Show resolved Hide resolved
@vcheplygina
Copy link
Contributor Author

I've fixed the lines now hopefully. For the image, it was meant to be a sketch and the only way I know how to create diagrams, I certainly wouldn't mind somebody replacing it with a more visually and technically pleasing version.

@ldko
Copy link
Contributor

ldko commented Dec 23, 2020

Thanks, @vcheplygina! I understand about the diagram. I have not created an svg from scratch myself. :)

@maxim-belkin @annefou , I am happy with these changes. If we drop the pptx file, does it sound alright to you all to merge these changes, and if someone later wants to do any work on the image, they could use the png as a guide and recreate and edit it as an svg?

@maxim-belkin
Copy link
Contributor

If we drop the pptx file, does it sound alright to you all to merge these changes, and if someone later wants to do any work on the image, they could use the png as a guide and recreate and edit it as an svg?

Yes, let's drop pptx and add alt. text before merging. This PR would have to be squash-merged (so that we don't bring in pptx from Veronika's branch).

Thank you, @vcheplygina, for your contribution!
And, of course, thank you, @ldko for taking care of this PR!

Co-authored-by: Maxim Belkin <maxim.belkin@gmail.com>
@maxim-belkin
Copy link
Contributor

Veronika, apologies for taking so long to get to this. I've updated the figure. We (@ldko, @annefou and myself) discussed it briefly and it looks like we all like it. We still need an alt text for it but we can address it in a separate PR. Once this PR is merged, I'll push an updated gh-pages branch to your fork to keep it in a pristine state.

Thank you very much for your contribution to the lesson!

@maxim-belkin maxim-belkin merged commit dfe8b1c into swcarpentry:gh-pages Jan 29, 2021
@maxim-belkin
Copy link
Contributor

Veronika, a quick update: GitHub rejected my attempt to update the gh-pages branch in your fork after the PR was merged. Here's what you need to do to sync your gh-pages branch with the one in this repo:

# navigate to the folder containing this lesson on your computer
git checkout gh-pages
git fetch https://github.com/swcarpentry/python-novice-inflammation
git reset --hard FETCH_HEAD

@vcheplygina
Copy link
Contributor Author

vcheplygina commented Jan 30, 2021 via email

@ldko
Copy link
Contributor

ldko commented Jan 30, 2021

Thanks @vcheplygina for proposing the new overview image and for clarifying the intro text. @maxim-belkin thanks for tidying up the image as an svg and getting things merged. I think the page looks great :).

zkamvar pushed a commit that referenced this pull request Apr 21, 2023
Co-authored-by: Maxim Belkin <maxim.belkin@gmail.com>
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.

3 participants