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

Modified dashboard page #141

Merged
merged 47 commits into from
Jul 21, 2019

Conversation

sanketbansal
Copy link
Contributor

@sanketbansal sanketbansal commented Jun 6, 2019

Changes proposed in this pull request:

  • Modified dashboard page UI to make it similar to the current EvalAI UI

Screenshot from 2019-06-06 00-30-19

@codecov-io
Copy link

codecov-io commented Jun 8, 2019

Codecov Report

Merging #141 into master will decrease coverage by 0.1%.
The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master     #141      +/-   ##
==========================================
- Coverage   52.17%   52.06%   -0.11%     
==========================================
  Files          61       61              
  Lines        3178     3196      +18     
  Branches      370      371       +1     
==========================================
+ Hits         1658     1664       +6     
- Misses       1436     1444       +8     
- Partials       84       88       +4
Impacted Files Coverage Δ
...onents/home/twitter-feed/twitter-feed.component.ts 100% <ø> (ø) ⬆️
src/app/components/nav/footer/footer.component.ts 63.63% <100%> (+2.34%) ⬆️
...d/dashboard-content/dashboard-content.component.ts 55% <55%> (ø)
...nents/nav/header-static/header-static.component.ts 77.96% <77.77%> (+3.82%) ⬆️
...rc/app/components/dashboard/dashboard.component.ts 87.5% <81.81%> (+7.01%) ⬆️
src/app/services/endpoints.service.ts 67.1% <0%> (-2.64%) ⬇️
src/app/services/mock.window.service.ts 100% <0%> (+16.66%) ⬆️
Impacted Files Coverage Δ
...onents/home/twitter-feed/twitter-feed.component.ts 100% <ø> (ø) ⬆️
src/app/components/nav/footer/footer.component.ts 63.63% <100%> (+2.34%) ⬆️
...d/dashboard-content/dashboard-content.component.ts 55% <55%> (ø)
...nents/nav/header-static/header-static.component.ts 77.96% <77.77%> (+3.82%) ⬆️
...rc/app/components/dashboard/dashboard.component.ts 87.5% <81.81%> (+7.01%) ⬆️
src/app/services/endpoints.service.ts 67.1% <0%> (-2.64%) ⬇️
src/app/services/mock.window.service.ts 100% <0%> (+16.66%) ⬆️

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 ffd35c9...99befb6. Read the comment docs.

@sanketbansal
Copy link
Contributor Author

@lunayach this can be reviewed now

angular.json Outdated
"src/styles/base.scss",
"./node_modules/froala-editor/css/froala_editor.pkgd.min.css",
"./node_modules/froala-editor/css/froala_style.min.css",
"./node_modules/ng-pick-datetime/assets/style/picker.min.css"
"./node_modules/ng-pick-datetime/assets/style/picker.min.css",
"src/styles/styles-port/basic.scss"
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we agree on removing styles-port?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am making a PR for that after merging that It will be removed. I kept it to make the PR functional.

*/
ngOnInit() {
if (!this.authService.isLoggedIn()) {
this.router.navigate(['/auth/login']);
Copy link
Member

Choose a reason for hiding this comment

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

Don't hard-code the URL, /auth/login, define a separate variable above or get from elsewhere if available.

/**
* Participant teams list
*/
participantteams = [];
Copy link
Member

Choose a reason for hiding this comment

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

Camel casing in variable names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

);
this.authServiceSubscription = this.authService.change.subscribe((authState) => {
if (!authState.isLoggedIn) {
this.router.navigate(['/auth/login']);
Copy link
Member

Choose a reason for hiding this comment

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

Same here for /auth/login.

@@ -0,0 +1,37 @@
<footer class="ev-md-container dashboard-footer ev-dark-bg">
Copy link
Member

Choose a reason for hiding this comment

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

I had reviewed the same file in #158!
Should we close/merge that first?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we should close that PR

@@ -165,4 +177,6 @@ export class HeaderStaticComponent implements OnInit, OnDestroy {
this.isMenuExpanded = !this.isMenuExpanded;
}

profileDropdown(event) {}

Copy link
Member

Choose a reason for hiding this comment

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

This function does nothing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i will remove that.

@@ -12,12 +12,21 @@ export class AuthService {


/**
* Modifications in Auth Services
* Ported From Angular Application
Copy link
Member

Choose a reason for hiding this comment

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

Means ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will remove these comments.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

},
err => {
error();
Copy link
Member

Choose a reason for hiding this comment

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

When this function will be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed changes in it

templateUrl: './simple-header.component.html',
styleUrls: ['./simple-header.component.scss']
})
export class SimpleHeaderComponent implements OnInit, OnDestroy {
Copy link
Member

Choose a reason for hiding this comment

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

Why its name is simple header ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I named it as per used in angularJS project.

})
export class SimpleHeaderComponent implements OnInit, OnDestroy {

user = {username: ''};
Copy link
Member

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 create an object then it would be better to user Angular interface concept and make a model in a separate file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will keep that in mind.

Ported the image assets from angularJs application into 'image-port' directory
Ported styles from angularjs Application inside style-port folder
Made the footer UI similar to the angularjs application one.
Made header UI similar to Angularjs Application one.
Added isAuth parameter in auth service
Added mock window service for calling loadJS method.
Added routerLink proprety for routing through UI
Added github fork and star button using github api
Changes made in auth services to integrate angularJS authentication features
Changes made in static header to handle different authentication cases
Added main.scss file of ported style in the angular.json file to compile it at the time of building project.
Dashboard-content component added to added functionality in the dashboard UI
Added proprtiess in .btn class
@sanketbansal sanketbansal force-pushed the modified-dashboard branch 2 times, most recently from 9625e1a to 3bfab54 Compare July 21, 2019 05:34
Unnecessary changes removed from auth service
@lunayach
Copy link
Member

@sanketbansal please verify the changes once before pushing. This big image comes when visiting any link in the application:

two

@sanketbansal
Copy link
Contributor Author

@lunayach it's working fine on my machine.

Screenshot from 2019-07-21 03-02-01

</div>
</div>
</div>
<footer class="ev-md-container ev-footer ev-dark-bg">
Copy link
Member

Choose a reason for hiding this comment

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

@sanketbansal no need of this footer component again. It has the identical code for dash-footer component which is already merged in #158.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, will remove that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should I rename the dash-footer to footer ?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, please.

<div class="btn ev-btn-dark waves-effect waves-dark grad-btn grad-btn-dark fs-14" (click)="logOut()">Logout</div>
</li>
</ul>
<ul class="side-nav" style="width:200px; transform:translateX(0px);" (click)="menuExpander()" *ngIf="!isMenuExpanded" >
Copy link
Member

Choose a reason for hiding this comment

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

Move these inline styles to .scss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

</ul>
<ul class="right hide-on-med-and-down">
<li *ngIf="!authService.isAuth"><a class="waves-effect waves-dark main-header-link" routerLink="/auth/signup" routerLinkActive="active">Sign Up</a></li>
<li *ngIf="!authService.isAuth"><a class="btn ev-btn-dark waves-effect waves-dark grad-btn grad-btn-dark fs-14" style="padding: 0 29px; font-size: 14px; display: inline-block" routerLink="/auth/login" routerLinkActive="active">Log In</a></li>
Copy link
Member

Choose a reason for hiding this comment

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

Here, as well, move adding: 0 29px; font-size: 14px; display: inline-block to .scss.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

@HostListener('window:resize', ['$event'])
onResize(event) {
this.innerWidth = window.innerWidth;
if (this.innerWidth <= 810) {
this.isMenuExpanded = false;
if (this.innerWidth > 810) {
Copy link
Member

Choose a reason for hiding this comment

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

Create an expandMenu( ) method to use it in both the places i.e. Line 113 and Line 127 respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay

<a class="btn ev-btn-dark waves-effect waves-dark grad-btn grad-btn-dark fs-14" routerLink="/auth/login">Log In</a>
</li>
</ul>
<ul class="side-nav" style="width:200px; transform:translateX(0px);" (click)="menuExpander()" *ngIf="!isMenuExpanded">
Copy link
Member

Choose a reason for hiding this comment

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

Move style="width:200px; transform:translateX(0px);" to .scss. (Also, at other places.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@lunayach it's working fine on my machine.
Screenshot from 2019-07-21 03-02-01

Yes, re-building it helped. But as you might see, there is something wrong in the Home-page UI. Would this get fixed in #140?

yes for sure.

@@ -0,0 +1,43 @@
<div class="sim-header padding-lr-header">
Copy link
Member

Choose a reason for hiding this comment

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

Simple Header and Header Static component are very similar. Can you tell why do we need both of them?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I will try to make a single component for both of them.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! That'll be great.

/**
* Header is transparent on these URLs
*/
transparentHeaderUrls = ['', '/'];
Copy link
Member

Choose a reason for hiding this comment

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

I don't think, this is used anywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay, I will check and remove that

@lunayach
Copy link
Member

@lunayach it's working fine on my machine.

Screenshot from 2019-07-21 03-02-01

Yes, re-building it helped. But as you might see, there is something wrong in the Home-page UI. Would this get fixed in #140?

UI bugs fixed in static header.
Simple header component has been removed
@lunayach lunayach mentioned this pull request Jul 21, 2019
Added checkInnerwidth method
Added styles of dash footer component
Dash footer has been removed as it has duplicate styles and html with footer component
@lunayach
Copy link
Member

@sanketbansal when I click on the available options on the side-bar (i.e. All Challenges, Hosted Challenges, etc.), the corresponding side-bar having these options dis-appears. (unlike current version). Can you please check this?

@sanketbansal
Copy link
Contributor Author

@sanketbansal when I click on the available options on the side-bar (i.e. All Challenges, Hosted Challenges, etc.), the corresponding side-bar having these options dis-appears. (unlike current version). Can you please check this?

That's not a bug actually after clicking on the options it gets routed to other components which are
being modified in different PR. There side bar remains intact.

@lunayach
Copy link
Member

@sanketbansal The stats for the GitHub are not displayed in one go and I have to do a refresh and only then they are displayed.

Added new github forks and star button
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 Now this can be merged.
It does break some UI of the homepage but that would be taken care with #140.

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

Successfully merging this pull request may close these issues.

5 participants