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

Science blog 6 (German) #3243

Merged
merged 22 commits into from
Nov 30, 2022
Merged

Conversation

brifemu
Copy link
Contributor

@brifemu brifemu commented Nov 23, 2022

PR to implement Science Blog 6 (German)

@brifemu brifemu requested a review from a team November 23, 2022 10:10
@mtb77 mtb77 self-requested a review November 23, 2022 10:14
Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

I've made some comments:

  1. Typos + one instance where I wasn't sure about the sentence.
  2. German abbreviations need a space in them (see DIN 5008).
  3. The link using ../ doesn't work. I suggest to use /de/science instead. Also you should check that the Cypress test npm test runs without errors.
  4. I mentioned also about the <figcaption> use not being compliant.

Copy link
Contributor

@Ein-Tim Ein-Tim left a comment

Choose a reason for hiding this comment

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

Three comments.

science/2022-11-23-science-blog-6/index.md Outdated Show resolved Hide resolved
science/2022-11-23-science-blog-6/index_de.md Outdated Show resolved Hide resolved
science/2022-11-23-science-blog-6/index_de.md Outdated Show resolved Hide resolved
mtb77 and others added 2 commits November 24, 2022 19:19
Co-authored-by: Tim <67682506+Ein-Tim@users.noreply.github.com>
Co-authored-by: Tim <67682506+Ein-Tim@users.noreply.github.com>
@brianebeling
Copy link
Member

I resolved the remaining suggestions - thanks! - and will hopefully commit those (visibly) next week. Right now they are on a fork of a fork, but I can't push directly and have to wait for @brifemu to accept the PR.

@mtb77
Copy link
Member

mtb77 commented Nov 28, 2022

@Ein-Tim feel free to have a final look at the changes :)

@Ein-Tim
Copy link
Contributor

Ein-Tim commented Nov 28, 2022

@mtb77 Will do so & provide feedback in ca. 60 minutes. Hope that's ok for you?

@mtb77
Copy link
Member

mtb77 commented Nov 28, 2022

@Ein-Tim sure, no rush. We would like to publish it Wednesday.

@MikeMcC399
Copy link
Contributor

Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

Apart from one missing comma, this PR is good to go!

Copy link
Contributor

@Ein-Tim Ein-Tim left a comment

Choose a reason for hiding this comment

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

Some comments.

science/2022-11-23-science-blog-6/index.md Outdated Show resolved Hide resolved
science/2022-11-23-science-blog-6/index_de.md Outdated Show resolved Hide resolved
science/2022-11-23-science-blog-6/index_de.md Outdated Show resolved Hide resolved
science/2022-11-23-science-blog-6/index_de.md Outdated Show resolved Hide resolved
science/2022-11-23-science-blog-6/index_de.md Outdated Show resolved Hide resolved
science/2022-11-23-science-blog-6/index_de.md Outdated Show resolved Hide resolved
Copy link
Contributor

@MikeMcC399 MikeMcC399 left a comment

Choose a reason for hiding this comment

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

It looks good to publish now!

Copy link
Contributor

@Ein-Tim Ein-Tim 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 now! Great work everybody, this was a really interesting read!

@mtb77 mtb77 merged commit a485aa5 into corona-warn-app:master Nov 30, 2022
@mtb77 mtb77 deleted the brifemu/science-blog-6 branch November 30, 2022 07:52
@MikeMcC399
Copy link
Contributor

It would be good in future to squash commits either before merging or as part of the merge. Otherwise the commit history in the master branch is much too detailed and is very untidy. There are 22 commits in this PR and they have each gone into the master branch individually instead of as just one consolidated commit. 🙁

See

@MikeMcC399
Copy link
Contributor

There is a working branch https://github.com/corona-warn-app/cwa-website/commits/pr/3243 related to this PR in this repository. Can the branch pr/3243 be deleted now that the PR is merged?

@HelTob
Copy link
Contributor

HelTob commented Dec 6, 2022

Hello everyone,

also from our side many thanks for your great work.
It makes us happy again and again to see with how much passion you work on our articles.

Keep up the good work and best regards,
Tobias

@MikeMcC399
Copy link
Contributor

Hi Tobias @HelTob !

It's also good to see how very useful the data donation has been and to see that you can show the usefulness of CWA exposure notification.

@MikeMcC399
Copy link
Contributor

@larswmh

There is an old unused branch pr/3243. Could you take a look to see if it can be deleted?

@larswmh
Copy link
Member

larswmh commented Jan 19, 2023

@MikeMcC399

Thanks for the information. I have deleted the branch.

@MikeMcC399
Copy link
Contributor

@larswmh

Thank you for cleaning up!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants