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

Modified homepage #140

Merged
merged 39 commits into from
Jul 21, 2019
Merged

Conversation

sanketbansal
Copy link
Contributor

@sanketbansal sanketbansal commented Jun 6, 2019

Changes proposed in this pull request:

  • Modified the home component to make its UI similar to current EvalAI UI.

Screenshot from 2019-06-05 22-01-50

Screenshot from 2019-06-05 22-01-58

Screenshot from 2019-06-05 22-02-05

Screenshot from 2019-06-05 22-02-20

@lunayach
Copy link
Member

lunayach commented Jun 6, 2019

@sanketbansal Could you see why the tests are failing?

@sanketbansal
Copy link
Contributor Author

Working on it

@codecov-io
Copy link

codecov-io commented Jun 8, 2019

Codecov Report

Merging #140 into master will increase coverage by 0.16%.
The diff coverage is 62.79%.

@@            Coverage Diff             @@
##           master     #140      +/-   ##
==========================================
+ Coverage   52.06%   52.23%   +0.16%     
==========================================
  Files          61       61              
  Lines        3196     3226      +30     
  Branches      371      374       +3     
==========================================
+ Hits         1664     1685      +21     
- Misses       1444     1451       +7     
- Partials       88       90       +2
Impacted Files Coverage Δ
src/app/services/auth.service.ts 24.05% <10%> (-0.31%) ⬇️
src/app/services/endpoints.service.ts 67.94% <100%> (+0.84%) ⬆️
src/app/components/home/home.component.ts 78.12% <77.41%> (-21.88%) ⬇️
src/app/services/window.service.ts 70.96% <0%> (-3.23%) ⬇️
Impacted Files Coverage Δ
src/app/services/auth.service.ts 24.05% <10%> (-0.31%) ⬇️
src/app/services/endpoints.service.ts 67.94% <100%> (+0.84%) ⬆️
src/app/components/home/home.component.ts 78.12% <77.41%> (-21.88%) ⬇️
src/app/services/window.service.ts 70.96% <0%> (-3.23%) ⬇️

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 8bb735a...b8da7d5. Read the comment docs.

@lunayach
Copy link
Member

@sanketbansal again 137 file changes! Are all of them for this PR only?

@sanketbansal
Copy link
Contributor Author

@lunayach No! it includes file changes of static styles and assets for which I have created a separate PR #154, #156, #157.
Please review those PRs first then I will rebase it.

@lunayach
Copy link
Member

@sanketbansal Yes, I have already reviewed #154. Please see.

@sanketbansal
Copy link
Contributor Author

@RishabhJain2018 @lunayach @Shekharrajak @galipremsagar please review

@lunayach
Copy link
Member

205 file changes! Need to review/merge #154, I guess.

@sanketbansal sanketbansal force-pushed the modified-homepage branch 3 times, most recently from bc51121 to 5b73a7e Compare July 19, 2019 04:11
@sanketbansal
Copy link
Contributor Author

@lunayach file changes has been reduced please review

@lunayach
Copy link
Member

@sanketbansal I see, that it still has styles-port folder?

@sanketbansal
Copy link
Contributor Author

Yeah I just kept it to show you the new base styles which needs to be add for the whole project as discussed. I will remove it after your review.

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

After resolving #141, @sanketbansal resolve conflicts here.

@lunayach
Copy link
Member

@sanketbansal large no. of conflicts are for this one; please resolve them.

Ported the image assets from angularJs application into 'image-port' directory
AngularJS home page ported to Angular 7 by converting main controller logic into Home Page class logic.
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.
Removed test cases to evaluate title and h1 element textContent
Removed unnecessary changes from home page
console logs removed from home page
Removed script to load github stars and forks.
Logic added for host challenge button to navigate challenge-create component
Assets removed from image-port folder and moved into image foider
Image assets moved from organization folder to partners folder
Styles from module folder removed
Duplicates removed from base.scss file
Duplicates removed from main.scss file
Unused css removed from basic .scss file
Styles relevant to component added
Unused methods removed from auth service
Added basic styles for base styles
Removed loading styles from basic.scss file
mian.scss file removed from styles-port folder
Removed styles from styles-port folder
</div>
</section>
<!-- organizatons section -->
<section class="ev-container ev-reverse-details text-med-black ev-super-light-bg " id="ev-scroll-down">
Copy link
Member

Choose a reason for hiding this comment

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

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

@@ -60,17 +60,4 @@ describe('HomeComponent', () => {
it('should be created', () => {
expect(component).toBeTruthy();
});

Copy link
Member

Choose a reason for hiding this comment

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

I see why you have removed the test specs here. But add to TODO's to add new test cases here.

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

if (this.authService.isAuth) {
this.router.navigate(['/challenge-create']);
} else {
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's; like in previous PR's, define them as constants above.

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

*/
ngOnInit() {}
init() {
const logInStatus = this.authService.isLoggedIn();
Copy link
Member

Choose a reason for hiding this comment

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

Where are we using logInStatus?

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

@@ -16,8 +16,14 @@ export class AuthService {
*/
isAuth = false;

/**
* Porting Ends
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

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

@@ -392,6 +393,10 @@ mat-chip {
padding: 30px 40px 40px 40px;
}

//.ev-md-container {
Copy link
Member

Choose a reason for hiding this comment

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

Remove this.

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

.btn-pagination:hover {
background-color: $highlight;
i {
font-size: 16px;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
font-size: 16px;
font-size: $fs-16;

Also, above.

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

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.

Functionality works! Minor changes needed.

@sanketbansal sanketbansal force-pushed the modified-homepage branch 2 times, most recently from ccd8c3f to b43c7a9 Compare July 21, 2019 17:04
Removed unused methods and variables
Removed commented lines.
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.

LGTM! @RishabhJain2018, Homepage is smooth now.

@RishabhJain2018 RishabhJain2018 merged commit c03b0cb into Cloud-CV:master Jul 21, 2019
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.

4 participants