-
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
Fellowship type pages #994
Conversation
a45fcd1
to
2bc979a
Compare
2bc979a
to
389c3a0
Compare
389c3a0
to
def5811
Compare
def5811
to
1b1dd55
Compare
@alanmoo this is ready for review! |
@@ -31,3 +31,29 @@ | |||
background-repeat: no-repeat; | |||
background-size: contain; | |||
} | |||
|
|||
.breadcrumb { |
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.
overriding some bootstrap's rules...
be06cb3
to
f55b029
Compare
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.
Some changes for the markup, though I'd like @gvn to take a look at the CSS
{% endblock %} | ||
|
||
{% block fellowship_description %} | ||
{% lorem 2 p %} |
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.
Neat
source/js/main.js
Outdated
name: `Firstname Surname`, | ||
role: `[Area Fellowship] Fellow, [Year]`, | ||
location: `City, Country`, | ||
image: `https://images.pexels.com/photos/264206/pexels-photo-264206.jpeg?w=500`, |
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 the secret gem of this PR
source/js/main.js
Outdated
}; | ||
|
||
ReactDOM.render(<Person metadata={metadata} />, document.getElementById(`featured-open-web-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.
This feels WETter than necessary...could the metadata be passed in as props on the target div (in the markup), and then render run on a generic class?
@@ -1,6 +1,6 @@ | |||
<!-- This is a copy/paste of landing-page.html, modified to be useful for fellowships to start. Ideally the entire site will extend this template eventually --> | |||
<!DOCTYPE html> | |||
<html>{% load settings_value %}{% load active_nav %} | |||
<html>{% load settings_value %}{% load fellowship_active_nav %} |
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 realize I'm a bit concerned with editing of this file too much to be specific to fellows (and still calling it foundation-base
). Perhaps we should abstract out a level so we can rely on a true "foundation base" generated from the original pug file? I'd say for this PR, let's start with renaming this file.
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.
k i'll rename it to fellowships_base.html
as the first step
source/js/main.js
Outdated
@@ -231,6 +232,42 @@ let main = { | |||
if (document.querySelector(`#news`)) { | |||
ReactDOM.render(<News env={env}/>, document.querySelector(`#news`)); | |||
} | |||
|
|||
// Science Fellowship |
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 we put this into a new file? main.js
is getting pretty large and this is fairly specific to just one section.
source/sass/type.scss
Outdated
text-decoration: none; | ||
border-bottom: 5px solid #ffed00; | ||
position: relative; | ||
top: 2.5px; |
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.
All browsers round subpixels differently, so its better to just use integer values with pixel units IMO.
There are some CSS tweaks I need to apply. Will flag someone to review after I'm done. |
function injectReactComponents() { | ||
// Science Fellowship | ||
if (document.getElementById(`featured-science-fellow`)) { | ||
let metadata = { |
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.
We could feed these hardcoded metadata via HTML data-*
attributes... but some of them aren't string. It's more troublesome to hardcode array and object as data-*
values... and it'll make the HTML code less readable. I can't find a DRYer way to do this... at least not now as these values are hardcoded.
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.
Now that this is in a separate file it makes a lot more sense.
@alanmoo this is ready for review again! |
} | ||
} | ||
|
||
export default { injectReactComponents }; |
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.
Exporting a named function here. I think we'll end up having more functions in this module in the future.
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 looks good in terms of code and a solid pass at populating the structure. Beyond content, the only glaring issue is the fellowships nav isn't responsive. Should we file that as another ticket @mmmavis?
function injectReactComponents() { | ||
// Science Fellowship | ||
if (document.getElementById(`featured-science-fellow`)) { | ||
let metadata = { |
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.
Now that this is in a separate file it makes a lot more sense.
@alanmoo re: nav I haven't really worked on styling yet since mockups aren't finalized yet. But yea I can file a ticket to make sure we don't forget to make the nav responsive. |
Related to #951
Updated comps: https://projects.invisionapp.com/share/ZJ9YTAGEA#/screens/273312988
https://foundation-mofostaging--pr-994.herokuapp.com/fellowships/science/
https://foundation-mofostaging--pr-994.herokuapp.com/fellowships/open-web/