-
Notifications
You must be signed in to change notification settings - Fork 153
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
Fellowships directory pages #1046
Conversation
@alanmoo assigned this one to your for review since you were already checking out this directory page work earlier |
@@ -23,13 +23,14 @@ import fellowships from './fellowships'; | |||
const SHOW_MEMBER_NOTICE = false; | |||
|
|||
// To be populated via XHR... | |||
let env, networkSiteURL; | |||
let env, networkSiteURL, pulseApiURL; | |||
|
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.
Is this going to cause problems when promoting heroku pipelines?
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.
Oh or is it pulling this from the environment json view that the server exposes?
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.
@alanmoo I think it should be fine. I'm following exactly how we are doing to get networkSiteURL
from the env var NETWORK_SITE_URL
(from environment json).
url(r'^directory/$', | ||
views.fellows_directory, | ||
name='fellowships-directory'), | ||
url(r'^directory/senior$', |
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.
I know we're doing things pretty statically, but I think it's worth it to make this DRYer. Check out named groups
req.send(); | ||
} | ||
|
||
function renderFellowCard(fellow) { |
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.
Can you put a comment in describing what this is doing (vs, say, the existing person.jsx
component)?
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.
This is looking pretty good, a few smaller comments beyond DRYing out the view logic
source/js/fellowships.js
Outdated
</button>; | ||
}; | ||
|
||
getFellows({'program_year': `2017`}, fellows => { |
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.
Let's make this a const at the top of the file so it stands out more when we want to change it
source/js/fellowships.js
Outdated
|
||
getFellows({'program_year': `2017`}, fellows => { | ||
let fellowsByType = groupFellowsByAttr(`program_type`, fellows); | ||
const ORDER = [ `senior`, `science`, `open web`, `tech policy`, `media`]; |
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.
Maybe bring this to the top while you're at it?
url(r'^directory/$', | ||
views.fellows_directory, | ||
name='fellowships-directory'), | ||
url(r'^directory/(?P<program_type_slug>[-\w]+)/$', | ||
views.fellows_directoy_type, | ||
name='fellowships-directory-senior'), |
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.
Why is this called "senior"?
@@ -80,7 +80,7 @@ | |||
<div class="col"> | |||
<div id="multipage-nav" class="d-flex flex-row align-items-center"> | |||
<div><a href="{% url 'fellowships-home' %}" class="{% fellowship_active_nav request 'fellowships-home fellowships-science fellowships-open-web' %}">Fellowships</a></div> | |||
<div><a href="{% url 'fellowships-directory' %}" class="{% fellowship_active_nav request 'fellowships-directory' %}">Directory</a></div> | |||
<div><a href="{% url 'fellowships-directory' %}" class="{% fellowship_active_nav request 'fellowships-directory fellowships-directory-senior' %}">Directory</a></div> |
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.
Similar to above, I'm not clear on what the senior
bit is doing here
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.
A few comments above, but nothing serious. This looks pretty good considering where the API has been. Once we've polished the API, I think we can clean up a lot more of this to be a bit more content-agnostic.
* Related to #955 - fellowships directory pages
* Set stage for fellowships pages w/ django template (#961) * Set stage for fellowships pages w/ django template * Adding some stub urls for the fellowships site. (#971) * Adding some stub urls for the fellowships site. * Update some of the urls a bit more * fellowships homepage wireframe (#965) * Fix review apps for fellows (#1010) * Test commit * Find out which url is it resolving to * Fix stuff * Remove debugging stuff * fixed rebase conflicts * Fellowships support page (#1006) Fellowships support page * fixed rebase conflicts * Responsive fellowships nav & better multi-page nav on mobile (#1020) * Related to #1019 - repsonsive fellowships nav + #977 - better multi-page nav on mobile * Updated fellowships homepage (#1042) * updated fellowships homepage * updated fellowships support page (#1049) * Fellowships directory pages (#1046) * Related to #955 - fellowships directory pages * Update fellowships type pages (#1059) * added illustration to fellowship types page and fellowships support page (#1061) * made fellowships work use the new master template (#1069) * updated /apply page so it's in 'application open' state (#1070) * More assets (#1073) * added more assets & created a svg sub-folder * added hero image to fellowships homepage * Related to #1062 - hide affiliation meta from front end (#1075) * design tweaks (#1081) * design tweaks * show fellow location in fellowships directory (#1103) * Added loading icon (#1093) * Related to #1045 - added loading icon * Featured fellows (#1112) * added featured fellows on fellowships homehomepomepage (hardcoded) * hide some incomplete content for now (#1113) * Fixes #1092 - added back missing icon & ran task to optimize svgs (#1108) * Added placeholder headshot to Person component (#1119) * Related #1114 - added placeholder headshot * Updated fellowships homepage content and layout again (#1122) * Related to #1120 - updated fellowships homepage content and layout again * Related to #1124 - hero banner adjustment * Fixed #1096 - replaced a svg (#1123) * Content changes to apply page (#1127) * Related to #1116 - content changes to apply page * Fixed broken 'see work' link (#1129) * Fixed #1128 - fixed broken 'see work' link * Fixes #1144 - fixed a fellow's social link (#1145) * remmoved 'senior fellow' section from directory pages (#1150) * redirected /fellowship to /fellowships (#1149) * redirected /fellowship to /fellowships * removed an extra line * Fellowships improvements (#1157) * code improvement * removed a few empty files
Related to #955
location
andprofile id
info. That's why the location field and "see work" link in the<Person>
cards don't work as expected.e.g.,
🔷 https://foundation-mofostaging-pr-1046.herokuapp.com/fellowships/directory/
🔷 https://foundation-mofostaging-pr-1046.herokuapp.com/fellowships/directory/<program_type>