Load Base Components only once by jayaike · Pull Request #233 · Cloud-CV/EvalAI-ngx

jayaike · 2019-12-09T17:15:17Z

Changes proposed in this pull request:

Load the base components like header and footer only once, instead of on every page load

…/EvalAI-ngx into Base-Component-Features

codecov-io · 2019-12-09T17:48:31Z

Codecov Report

Merging #233 into master will decrease coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #233      +/-   ##
==========================================
- Coverage   51.73%   51.68%   -0.06%     
==========================================
  Files          66       66              
  Lines        3659     3659              
  Branches      413      413              
==========================================
- Hits         1893     1891       -2     
- Misses       1671     1672       +1     
- Partials       95       96       +1

Impacted Files	Coverage Δ
src/app/components/nav/footer/footer.component.ts	`57.57% <0%> (-6.07%)`	⬇️

Impacted Files	Coverage Δ
src/app/components/nav/footer/footer.component.ts	`57.57% <0%> (-6.07%)`	⬇️

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 b03bdc4...d7b5f33. Read the comment docs.

lunayach · 2019-12-10T13:08:48Z

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


There should only be 1-2 line changes. Other changes are due to spaces, indentation etc. Please remove unnecessary changes.

The reason for the line changes is I had to format the code after removing the footer.

lunayach · 2019-12-10T13:19:51Z

You may want to check the conditions correctly when the user is logged in. For example, when I am logged in, in the dashboard, two footers are appearing:

jayaike · 2019-12-10T13:43:23Z

I have removed the extra footer

jayaike · 2019-12-10T14:17:08Z

I also noticed there was an extra header in the dashboard so I removed that too

pushkalkatara · 2019-12-10T21:05:52Z

Hi @nsjcorps Please remove all the extra lines which are left after removing the header and footer from individual components and format the files.

pushkalkatara · 2019-12-10T21:06:38Z

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


Please format the file

The file is already formatted. See the screenshot below. I guess what you are seeing is the second to the last line.

pushkalkatara · 2019-12-10T21:06:58Z

 <div class="clearfix"></div>

-<app-footer *ngIf="!authService.isAuth"></app-footer>
+


please remove the extra line.

I have done that now

I also made some more changes @lunayach which I have explained below. Please let me know your opinion

lunayach · 2019-12-11T10:53:32Z

  </div>
 </div>
-
-<app-footer *ngIf="!authService.isAuth"></app-footer>


So now we do not need to check these conditions?

lunayach · 2019-12-11T11:00:24Z

@nsjcorps, It would be great if you could review the UI on your side before pushing the PRs.

Since you permanently added <app-header-static></app-header-static> and <app-footer></app-footer> for all the components, something like this is happening,

and,

jayaike · 2019-12-11T11:23:29Z

Ok thanks, but how will I do it in this case since all components inherit from the base component

jayaike · 2019-12-11T11:30:22Z

How would I test the condition now that it is in the main module?

jayaike · 2019-12-11T11:39:26Z

Should I use the URL as a test?

jayaike · 2019-12-11T12:00:10Z

Here is what I have concluded with.

I will give the sidebar a fixed color identical to the background.
That way it will overlay the footer and still not cut off any content.

Then as for the header, I will set a condition to check if the URL contains "auth".
If it does, it should not show.

How does it look to you? Please let me know your opinion.

Thank you, it's really fun contributing to EvalAI.

jayaike · 2019-12-11T12:03:26Z

For the header, I used "auth" because auth is the base URL for the components that don't use the header.

jayaike · 2019-12-11T15:42:22Z

Let me give a break down of my changes.

For the issue with the header:
I first subscribed to the auth events filtering out the NavigationEnd to see where the URL stops.
I then tested to see if the route had 'auth' in them; if they did it should not show the header as that means it is either on the login page, the signup page, the reset password page, etc.
Hence, it should not show the header.

For the issue of the footer:
When the user is logged in (authenticated) it shows the footer that accommodates the sidebar.
When the user is not logged in it shows the full-length footer as it should.

All of this was accomplished while still loading the components just once instead of on every page.

I hope this is a good fix. Please let me know if you have any comments.

It took some thinking ut I believe this is an optimal solution.

jayaike · 2019-12-11T15:46:26Z

I made 'authService' public so as to make it accessible it in app.component.html

lunayach · 2019-12-12T17:08:59Z

@nsjcorps Thanks a lot for making changes. And, great job thinking through the solution. Checking for URL is nice but feels hardcoded, and we should avoid that. Making sidebar's colour identical to the background also feels hacky.

lunayach · 2019-12-12T17:29:24Z

@nsjcorps I guess this is more complex than we initially thought. It might end up making the code-base more complicated than simplifying it. If you don't mind, maybe you could try solving other tasks, and meanwhile, we'll try to make this task more concrete with more specific objectives. Happy Hacking!! 😄

jayaike · 2019-12-12T18:11:58Z

@lunayach , Alright not a problem. Thanks

jayaike added 6 commits December 9, 2019 18:09

Removed Base Components (Header and Footer)

d9188da

Added Base Components (Header and Footer)

e9e72de

Merge branch 'master' into Base-Component-Features

6a3b32b

Formatted Code

7476a5a

Removed condition on Footer

25ed8d2

Merge branch 'Base-Component-Features' of https://github.com/nsjcorps…

9d34916

…/EvalAI-ngx into Base-Component-Features

lunayach reviewed Dec 10, 2019

View reviewed changes

Removed footer

9f17c42

Removed Header

8165718

pushkalkatara reviewed Dec 10, 2019

View reviewed changes

Formatted Code

d7b5f33

lunayach suggested changes Dec 11, 2019

View reviewed changes

jayaike added 3 commits December 11, 2019 16:33

Adjusted footer condition

c91e800

Fixed issues with auth routes

8424cc3

Fixed issues with auth routes

8b3fb07

Made authService public

421b295

		<div class="clearfix"></div>

		<app-footer *ngIf="!authService.isAuth"></app-footer>

Conversation

jayaike commented Dec 9, 2019

Changes proposed in this pull request:

Uh oh!

codecov-io commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lunayach commented Dec 10, 2019

Uh oh!

jayaike commented Dec 10, 2019

Uh oh!

jayaike commented Dec 10, 2019

Uh oh!

pushkalkatara commented Dec 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lunayach commented Dec 11, 2019

Uh oh!

jayaike commented Dec 11, 2019

Uh oh!

jayaike commented Dec 11, 2019

Uh oh!

jayaike commented Dec 11, 2019

Uh oh!

jayaike commented Dec 11, 2019

Uh oh!

jayaike commented Dec 11, 2019

Uh oh!

jayaike commented Dec 11, 2019

Uh oh!

jayaike commented Dec 11, 2019

Uh oh!

lunayach commented Dec 12, 2019

Uh oh!

lunayach commented Dec 12, 2019

Uh oh!

jayaike commented Dec 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Dec 9, 2019 •

edited

Loading