Skip to content
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

closed #60 & #61 #66

Closed
wants to merge 3 commits into from
Closed

closed #60 & #61 #66

wants to merge 3 commits into from

Conversation

rishiraj824
Copy link
Member

@mariobehling Please check the commit for the issues #60 & #61 as assigned. Thank You.

@mariobehling
Copy link
Member

Please ensure build tests are passing. Travis will pass then.

@hemantjadon
Copy link
Contributor

@rishiraj824 errors are due to you don't have the latest code in your fork.
Please check :)

@rishiraj824
Copy link
Member Author

Okay will check. Thanks.

@hemantjadon
Copy link
Contributor

@rishiraj824 also for quick test you can run ng test and ng lint locally

@rishiraj824
Copy link
Member Author

@mariobehling Please check the latest commit on the master branch. There shouldn't be any errors now.
this screenshot is of ng lint and ng test
screenshot from 2016-10-23 16 47 02
screenshot from 2016-10-23 16 09 35
This is the About Page #60
screenshot from 2016-10-23 16 08 59
This is the contact page #61
screenshot from 2016-10-23 16 54 27

@rishiraj824
Copy link
Member Author

rishiraj824 commented Oct 23, 2016

The errors in travis are from the home and the feed components.

@mariobehling
Copy link
Member

mariobehling commented Oct 23, 2016

@hemantjadon Please help to resolve checks.

@rishiraj824 Please provide a link to a test site.

There is some text in red/pinkish color. These are supposed to link to internal search results e.g. #OpenData -- links to --> /search?query=#OpenData

@hemantjadon
Copy link
Contributor

hemantjadon commented Oct 23, 2016

@rishiraj824
I fixed these linting errors in last commit of mine.
Please safely pull that commit:

  • Make a branch <branch-name> so that all your changes are safe.
  • Then refer to this https://help.github.com/articles/syncing-a-fork/ for syncing your master branch with the main repo
  • merge <branch-name> to master locally, reslove conflicts if any.
  • push the changes.

These should resolve all the linting errors
Cheers :)

Copy link
Contributor

@hemantjadon hemantjadon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please move styles to <component-name>.component.scss rather than just below the html for uniformity in code base

</div>
</div>
<app-footer></app-footer>
<style type="text/css">
Copy link
Contributor

@hemantjadon hemantjadon Oct 23, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move these To about.component.scss

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay

</div>
<app-footer></app-footer>

<style type="text/css">
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move these to contact.component.scss

@mariobehling
Copy link
Member

@rishiraj824 Any questions regarding this issue at the moment?

@rishiraj824
Copy link
Member Author

@mariobehling i fixed the issues #60 & #61 already. Please check my last commit. I thought the changes were merged.

@rishiraj824
Copy link
Member Author

screenshot from 2016-10-27 21 01 13
ng lint test on my local

@mariobehling
Copy link
Member

Ok, closing it. I am following up about more details in the related issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants