Skip to content
This repository has been archived by the owner on Jul 6, 2020. It is now read-only.

Added dash footer component #158

Merged
merged 5 commits into from
Jul 20, 2019

Conversation

sanketbansal
Copy link
Contributor

@sanketbansal sanketbansal commented Jun 22, 2019

Changes proposed in this pull request:

  • Dash footer added for improving UI of Dashboard related components.

dash-footer

@codecov-io
Copy link

codecov-io commented Jun 22, 2019

Codecov Report

Merging #158 into master will increase coverage by 0.1%.
The diff coverage is 100%.

@@            Coverage Diff            @@
##           master     #158     +/-   ##
=========================================
+ Coverage   52.19%   52.29%   +0.1%     
=========================================
  Files          60       61      +1     
  Lines        3148     3155      +7     
  Branches      367      367             
=========================================
+ Hits         1643     1650      +7     
  Misses       1421     1421             
  Partials       84       84
Impacted Files Coverage Δ
...omponents/nav/dash-footer/dash-footer.component.ts 100% <100%> (ø)
Impacted Files Coverage Δ
...omponents/nav/dash-footer/dash-footer.component.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 99dc91a...8e47342. Read the comment docs.

@lunayach
Copy link
Member

@sanketbansal for all these PR's, since the Surge is not working currently, screenshots may help us to see the visual changes that you have made.

@sanketbansal
Copy link
Contributor Author

@lunayach
Copy link
Member

@sanketbansal resolve the conflicts.

@lunayach
Copy link
Member

@sanketbansal I think that this is also not reflected. You need to call the component selector somewhere.

@sanketbansal sanketbansal force-pushed the Added-Dash-Footer branch 2 times, most recently from ea8d3a7 to 6298ef2 Compare July 18, 2019 12:07
@sanketbansal
Copy link
Contributor Author

It is being used by the #141 #149 #143

Copy link
Member

@lunayach lunayach left a comment

Choose a reason for hiding this comment

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

@RishabhJain2018 we can also merge this now. We can scrutinize the reflection in UI when reviewing #141.

@RishabhJain2018
Copy link
Member

@sanketbansal Please resolve the conflicts here.

@sanketbansal
Copy link
Contributor Author

Done

@RishabhJain2018
Copy link
Member

@sanketbansal I think you forgot to push the changes.

Dash footer added for improving dashboard UI
Styles added into the base.scss and dash-footer component
Removed logs and comments from dash footer component
@sanketbansal
Copy link
Contributor Author

@sanketbansal I think you forgot to push the changes.

Yeah sorry Done now

Copy link
Member

@Shekharrajak Shekharrajak left a comment

Choose a reason for hiding this comment

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

I am not seeing the footer as the screenshot attached. Can you check in https://pr-158-evalai.surge.sh/

this.year = new Date().getFullYear();
const js = this.document.createElement('script');
js.src = 'https://buttons.github.io/buttons.js';
this.document.getElementsByTagName('head')[0].appendChild(js);
Copy link
Member

Choose a reason for hiding this comment

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

We should avoid using plain javascript in our Angular application. Use typescript and Angular features.

Copy link
Member

Choose a reason for hiding this comment

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

@sanketbansal so you want to add buttons.js to use class github-button , right ?

Copy link
Member

Choose a reason for hiding this comment

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

You can simply add script tag in html file. But I don't think you should import whole js file just to get github icon. Isn't we are already using font-awesome that has github stars and fork icons?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's not only github icons actually it is used to fetch number of stars and forks on the github repository also.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just want to let you know: If you want to manipulate any DOM element (the similar thing that you were doing -- accessing any tag) Angular have ElementRef , ViewRef, .. reference types to pick any DOM element and modify it. There is Renderer to create new elements as well.

Thanks for letting me know. I tried that but I can not get access to head tag do not know why?
Maybe because it's not a component specific element. what do you think?

@lunayach lunayach mentioned this pull request Jul 20, 2019
@Shekharrajak
Copy link
Member

I didn't understand why did you use plain javascript instead of Angular typescript code.

@sanketbansal
Copy link
Contributor Author

sanketbansal commented Jul 20, 2019

I didn't understand why did you use plain javascript instead of Angular typescript code.

I did not find any other method to insert script tag in the head tag.
As I am not appending inside the component-specific element.

@sanketbansal
Copy link
Contributor Author

@Shekharrajak is there a way to do it?

@Shekharrajak
Copy link
Member

@Shekharrajak is there a way to do it?

Commented #158 (comment)

Added script tag in index.html file
Removed javascript to append script tag into head element
@sanketbansal
Copy link
Contributor Author

@Shekharrajak is it okay now

@Shekharrajak
Copy link
Member

Just want to let you know: If you want to manipulate any DOM element (the similar thing that you were doing -- accessing any tag) Angular have ElementRef , ViewRef, .. reference types to pick any DOM element and modify it. There is Renderer to create new elements as well.

@RishabhJain2018 RishabhJain2018 merged commit 0acecfa into Cloud-CV:master Jul 20, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants