-
Notifications
You must be signed in to change notification settings - Fork 469
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
[WIP] Feature/add community groups #536
base: source
Are you sure you want to change the base?
[WIP] Feature/add community groups #536
Conversation
<link rel="stylesheet" href="/style.css"> | ||
<link rel="stylesheet" href="/community-groups.css"> | ||
<link rel="shortcut icon" href="/favicon.ico"> | ||
<link href="http://fonts.googleapis.com/css?family=Source+Sans+Pro:400,700|Source+Code+Pro" rel="stylesheet" type="text/css"> |
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.
replace with <link href="//fonts.googleapis.com/css?family=Source+Sans+Pro:400,700|Source+Code+Pro" rel="stylesheet" type="text/css">
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.
Yah, should have rebased yesterday
<div class="two-thirds"> | ||
<ul> | ||
<li><strong data-i18n="footer-contact-header">Contact</strong></li> | ||
<li><a href="https://twitter.com/nodeschool" target="_blank">t/@nodeschool</a></li> |
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 should use rel="noreferrer noopener"
on _blank
targets for security reasons
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 think we should fix this sooner, see #538
<ul> | ||
<li><strong data-i18n="footer-contact-header">Contact</strong></li> | ||
<li><a href="https://twitter.com/nodeschool" target="_blank">t/@nodeschool</a></li> | ||
<li><a href="https://github.com/nodeschool" target="_blank">gh/nodeschool</a></li> |
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.
same as L58
</ul> | ||
<ul> | ||
<li><strong data-i18n="footer-contribute-header">Contribute</strong></li> | ||
<li><a href="https://github.com/nodeschool/discussions/issues/new" target="_blank" data-i18n="footer-contribute-question">Open an Issue</a></li> |
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.
same as L58
<ul> | ||
<li><strong data-i18n="footer-contribute-header">Contribute</strong></li> | ||
<li><a href="https://github.com/nodeschool/discussions/issues/new" target="_blank" data-i18n="footer-contribute-question">Open an Issue</a></li> | ||
<li><a href="https://github.com/nodeschool/discussions/issues" target="_blank" data-i18n="footer-contribute-answer">Answer a Question</a></li> |
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.
same as L58
"organizers": ["hollomancer"], | ||
"website": "", | ||
"twitter": "", | ||
"repo": "http://github.com/nodeschool/military-community-group" |
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.
repo should point to https version by default
const mkdirp = require('mkdirp') | ||
|
||
function groupByValue(list, grouper, groupName) { | ||
var grouped = {} |
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.
any reason why we choose to use semicolons for some portions but not for others?
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.
See #537
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 goes beyond this PR imho
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 agree. thanks for opening #537. we should follow one style
I think it is not necessary (feasible) to wait for the translations before we can merge this. |
f459fc5
to
308a584
Compare
308a584
to
76b232e
Compare
#533
Translations