-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
fix(framework-examples): fix Aurelia example (#120) #126
Conversation
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.
Thanks for your work on this @Thanood! Changes requested are mostly nits.
# 2 space indentation | ||
[**.*] | ||
indent_style = space | ||
indent_size = 2 |
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.
nit: newline at EOF
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 hope this file is okay now, although GitHub issue tracker doesn't seem to recognize the newline I put in, it's there in my editor (and in GitHub's editor).
@@ -0,0 +1,4 @@ | |||
// Place your settings in this file to overwrite default and user settings. |
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 valid JSON?
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.
Well no, of course not. :)
It's what vscode puts in this file, sorry.
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.
np as long as whatever is using it can parse it, it's cool by me
@@ -2,4 +2,4 @@ | |||
interface URLSearchParams {} | |||
declare module "isomorphic-fetch" { | |||
export = fetch; | |||
} | |||
} |
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.
nit: newline at EOF
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 newline taken care of as well?
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.
Yes, it is. No idea why it doesn't show up.
"aurelia": { | ||
"build": { | ||
"resources": [] | ||
"repository": { |
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 add "private": true
here, as well as remove keywords
, author
, license
, contributors
, bugs
, and homepage
keys.
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.
Agreed. Is the repository
field okay?
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.
Ah yeah I'd take that out as well. Pretty much anything that has to do with the Aurelia Skeleton, since it doesn't help convey the fact that this is an MDC-Web example.
@@ -1,8 +1,10 @@ | |||
import {bindable, customAttribute, inject, DOM} from 'aurelia-framework'; | |||
|
|||
// Use webpack's require function to load the css | |||
const MDC_BUTTON_STYLES = require('mdc-button-styles'); | |||
DOM.injectStyles(MDC_BUTTON_STYLES); | |||
// const MDC_BUTTON_STYLES = require('mdc-button-styles'); |
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.
remove commented-out code?
// const MDC_BUTTON_STYLES = require('mdc-button-styles'); | ||
import '@material/button/dist/mdc.button.css'; | ||
|
||
// DOM.injectStyles(MDC_BUTTON_STYLES); |
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.
remove commented-out code?
@@ -1,8 +1,10 @@ | |||
import {bindable, customAttribute, inject, DOM} from 'aurelia-framework'; | |||
|
|||
// Use webpack's require function to load the 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.
is this comment still relevant?
@@ -2,10 +2,13 @@ import {inject, bindable, bindingMode, DOM} from 'aurelia-framework'; | |||
|
|||
// Since we don't have typings (yet) we require mdc-checkbox manually. | |||
// const MDCCheckboxModule = require('mdc-checkbox'); | |||
const {MDCCheckbox} = require('mdc-checkbox'); | |||
// const {MDCCheckbox} = require('mdc-checkbox'); |
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.
remove commented-out code?
const {MDCCheckbox} = require('mdc-checkbox'); | ||
// const {MDCCheckbox} = require('mdc-checkbox'); | ||
import {MDCCheckbox} from '@material/checkbox'; | ||
|
||
// Use webpack's require function to load the 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.
is this comment still relevant?
@@ -2,10 +2,13 @@ import {bindable, customAttribute, inject, DOM} from 'aurelia-framework'; | |||
|
|||
// Since we don't have typings (yet) we require mdc-ripple manually. |
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 comments as above regarding stale doc comments and commented-out code
Thanks, @traviskaufman. |
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.
LGTM, just one comment regarding package.json. Once that's fixed, good to merge.
"aurelia": { | ||
"build": { | ||
"resources": [] | ||
"repository": { |
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.
Ah yeah I'd take that out as well. Pretty much anything that has to do with the Aurelia Skeleton, since it doesn't help convey the fact that this is an MDC-Web example.
@@ -0,0 +1,4 @@ | |||
// Place your settings in this file to overwrite default and user settings. |
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.
np as long as whatever is using it can parse it, it's cool by me
@@ -2,4 +2,4 @@ | |||
interface URLSearchParams {} | |||
declare module "isomorphic-fetch" { | |||
export = fetch; | |||
} | |||
} |
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 newline taken care of as well?
Removed repo from package.json. 😃 |
Thanks as always @Thanood 👍 |
should fix #120