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

Newsletter #86

Merged
merged 3 commits into from
Feb 4, 2019
Merged

Conversation

evanhearne
Copy link
Collaborator

Description

Newsletter Feature added to the website.

What

January 2019 Newsletter is available.

Why

Parents can now see Newsletters online

Verification Steps

http-server it, see if Newsletter is present on the navbar. Page should show up with dropdown to Jan 2019 newsletter

Checklist:

  • This pr is linked to the related trello card
  • My code follows the style guidelines of this project
  • I have requested a code review from (at least) two team members
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • This change has been verified by another team member

@evanhearne
Copy link
Collaborator Author

@laurafitzgerald can you please review this when you have the time? :)

laurafitzgerald
laurafitzgerald previously approved these changes Feb 4, 2019
Copy link
Contributor

@laurafitzgerald laurafitzgerald left a comment

Choose a reason for hiding this comment

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

@evanhearne This looks great. The pages are working as expected. The only feedback would be to maybe chat with the stakeholders about the layout of the landing newsletter page where you have the drop down. We could perhaps make that look a little nicer. Functionally this works great! Couple of issues with the git commits in the pr but let's address that in an upcoming session. I think we can go over that again. If you want to try to squash the 8 commits you can do that. But I will leave that up to you. For a change like this it would make sense to only have 1 commit there.

changed preview link

added newsletter
@evanhearne
Copy link
Collaborator Author

@laurafitzgerald I squashed the commits, but for some reason it says I dismissed your stale review. I found this link that supposedly tells you to fix this. isaacs/github#1157 (comment) Meanwhile could you reapprove this branch so I can merge?

Copy link
Contributor

@laurafitzgerald laurafitzgerald left a comment

Choose a reason for hiding this comment

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

@evanhearne I see that you've squashed the commits now. I double checked it's still all working. Feel free to merge. 👍

@laurafitzgerald
Copy link
Contributor

@evanhearne I put a new setting over the weekend to dismiss an approval review on a force push. It's just another security approach in order to get changes verified again once a force push happens, just in case there is some problem with the force push.

@evanhearne
Copy link
Collaborator Author

@laurafitzgerald thanks for that. Merging now. :)

@evanhearne evanhearne merged commit 067839d into MountSionCBSSecondary:master Feb 4, 2019
@evanhearne evanhearne deleted the newsletter branch February 4, 2019 11:47
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