Add of partner page #53

anishaswain · 2017-04-30T16:44:13Z

Fixes issue #30

Features

Responsive in for all screen width
Gives a list of all organization, partner with HospitalRun

cc: @tangollama @jglovier

ghost · 2017-04-30T16:44:18Z

Your pull request doesn't follow our guidelines. Please fix the following:

Pull request title must start with uppercase (?)
Pull request title must be in imperative, present tense (e.g. "add", "fix", "change") (?)

Click here for details.

Thank you! 🙏
_{This comment was made by GitMagic – Magically enforcing your contribution guidelines.}

ghost · 2017-04-30T16:48:41Z

This pull request still violates some of our guidelines:

Pull request title must be in imperative, present tense (e.g. "add", "fix", "change") (?)

Click here for details.

ghost · 2017-04-30T16:51:38Z

Thank you, the title and description now looks good!

jglovier

Thanks for getting this going @anisha1234! Left a few inline comments below:

jglovier · 2017-05-01T16:29:15Z

_layouts/partners.html

+  {% include partners-list.html %}
+</section>
+<center class="partner-heading">
+    <h1>Hopefully Soon</h1>


Please remove this list from the page. I know @tangollama included those on his list in the issue, but we shouldn't display orgs that aren't actually partners on this page.

jglovier · 2017-05-01T16:31:14Z

_includes/partners-list.html

@@ -0,0 +1,30 @@
+<center class='each-partner'>
+  <div class="partner-post">
+    <img src="../img/cure-logo.png" class="partner-logo" >


Each of these image elements needs an alt tag describing the image for accessibility. Also, they should have a width attribute to prevent the layout from jumping around as images load for users with slower connections.

jglovier · 2017-05-01T16:36:14Z

_sass/_partners.scss

+    }
+    .partner-logo{
+      width: 200px;
+      height: 8vw;


The use of a defined width but a relative height on these two lines above is causing the proportions of the logos to rendered as skewed. Instead, you could simply remove these properties and set a width attribute only directly on the img elements in the markup.

jglovier · 2017-05-01T16:37:08Z

_sass/_partners.scss

+@media screen and (min-width: 280px) {
+    .each-partner {
+        width: 100%;
+        background-color: #273647;


I'd prefer to see these not boxed in, and using the one color versions of the logos. However, if that's going to be challenging because of logo availability, let's just use a white background here #ffffff.

Also, if we're going to use the boxes, please use border-radius on them for consistency with other image/box elements we use on the site.

jglovier · 2017-05-01T16:38:52Z

_includes/partners-list.html

+  </div>
+  <h3 class="partner-name">Cure International</h3>
+</center>
+<center class='each-partner'>


The <center> should not be used, as it was a presentational element and has been deprecated from the HTML spec.

jglovier · 2017-05-01T16:41:20Z

_sass/_partners.scss

+    .partner-logo{
+      width: 200px;
+      height: 8vw;
+      padding: 1%;


I'd prefer to stick with consistent units in our CSS, so for padding and margin let's keep these all as px units, rather than percentage values or vw.

anishaswain · 2017-05-01T18:21:19Z

@jglovier Thanks for your review. It is really helpful .I will start working on the suggested changes as soon as possible.

anishaswain · 2017-10-23T18:27:39Z

@jglovier @tangollama Sorry for the delay. I have worked on the suggested changes and made required modifications. Kindly review the PR once again and let me know if it requires improvements.

jglovier

Thanks for making some changes @anisha1234. I've reviewed the code and have a few more changes to request please, mostly around the images again.

I realized that some of the images you have are themselves skewed, or not the original logo. For example, the CURE logo is squished horizontally, as is the eHealth Africa logo, and the New Relic logo. Also, The GitHub logo is also squished, and per GitHub branding guidelines should not include the octocat along with the wordmark.

Could you please resolve these issues with the partner logos?

jglovier · 2017-10-24T16:05:31Z

_includes/partners-list.html

@@ -0,0 +1,30 @@
+<div style="text-align:center" class='each-partner'>
+  <div class="partner-post">
+    <img src="../img/cure-logo.png" alt="Cure International logo" class="partner-logo">


These image tags are still missing an inline width attribute.

jglovier · 2017-10-24T16:05:50Z

_includes/partners-list.html

+</div>
+<div style="text-align:center" class='each-partner'>
+  <div class="partner-post">
+    <img src="../img/eHealth.png" alt="eHealthAfrica logo" class="partner-logo" >


...same here. Needs width attribute.

jglovier · 2017-10-24T16:05:57Z

_includes/partners-list.html

+</div>
+<div style="text-align:center" class='each-partner'>
+  <div class="partner-post">
+    <img src="../img/msbp.png" alt="Microsoft's Bizspark logo" class="partner-logo" >


...same here. Needs width attribute.

jglovier · 2017-10-24T16:06:03Z

_includes/partners-list.html

+</div>
+<div style="text-align:center" class='each-partner center'>
+  <div class="partner-post">
+    <img src="../img/newrelic.png" alt="New Relic logo" class="partner-logo" >


...same here. Needs width attribute.

jglovier · 2017-10-24T16:06:08Z

_includes/partners-list.html

+</div>
+<div style="text-align:center" class='each-partner center'>
+  <div class="partner-post">
+    <img src="../img/ember.svg" alt="Ember Sherpa logo" class="partner-logo" >


...same here. Needs width attribute.

jglovier · 2017-10-24T16:07:07Z

_sass/_partners.scss

+    }
+    .partner-logo{
+      width: 200px;
+      height: 100px;


Please remove the height attribute from this line, and set the width in the line above to max-width: 100%; instead. That will make it responsive within the container.

anishaswain · 2017-10-25T07:30:08Z

@jglovier I have made the modifications. Kindly verify if it needs further modifications.

jglovier

Looks good! Thanks for attending to all the changes @anisha1234!! 👏 ❤️ ⚡

addition of partner page

3f9bf27

anishaswain changed the title ~~addition of partner page~~ Addition of partner page Apr 30, 2017

anishaswain changed the title ~~Addition of partner page~~ Adding of partner page Apr 30, 2017

anishaswain changed the title ~~Adding of partner page~~ Add of partner page Apr 30, 2017

anishaswain mentioned this pull request Apr 30, 2017

Partners page #30

Closed

tangollama self-assigned this Apr 30, 2017

Merge branch 'master' into master

5e4889b

jglovier suggested changes May 1, 2017

View reviewed changes

jglovier mentioned this pull request May 8, 2017

Prioritization #31

Closed

7 tasks

ANISHA SWAIN added 3 commits October 23, 2017 16:51

Merge branch 'master' into master

4fe4648

changes as requested

79c98c9

Merge branch 'master' of github.com:Anisha1234/hospitalrun.github.io

fb7594c

jglovier suggested changes Oct 24, 2017

View reviewed changes

Anisha Swain added 2 commits October 25, 2017 12:31

changed logo

b2ad507

changed logo2

818e3ef

jglovier approved these changes Oct 30, 2017

View reviewed changes

jglovier merged commit 24bb381 into HospitalRun:master Oct 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add of partner page #53

Add of partner page #53

anishaswain commented Apr 30, 2017 •

edited

ghost commented Apr 30, 2017

ghost commented Apr 30, 2017

ghost commented Apr 30, 2017

jglovier left a comment

jglovier May 1, 2017

jglovier May 1, 2017

jglovier May 1, 2017

jglovier May 1, 2017

jglovier May 1, 2017

jglovier May 1, 2017

jglovier May 1, 2017

anishaswain commented May 1, 2017

anishaswain commented Oct 23, 2017

jglovier left a comment

jglovier Oct 24, 2017

jglovier Oct 24, 2017

jglovier Oct 24, 2017

jglovier Oct 24, 2017

jglovier Oct 24, 2017

jglovier Oct 24, 2017

anishaswain commented Oct 25, 2017

jglovier left a comment

Add of partner page #53

Add of partner page #53

Conversation

anishaswain commented Apr 30, 2017 • edited

ghost commented Apr 30, 2017

ghost commented Apr 30, 2017

ghost commented Apr 30, 2017

jglovier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anishaswain commented May 1, 2017

anishaswain commented Oct 23, 2017

jglovier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anishaswain commented Oct 25, 2017

jglovier left a comment

Choose a reason for hiding this comment

anishaswain commented Apr 30, 2017 •

edited