Cloud-CV / EvalAI-ngx

Revamped codebase of EvalAI Frontend
BSD 3-Clause "New" or "Revised" License
33 stars 61 forks source link

Load Base Components only once #233

Open jayaike opened 4 years ago

jayaike commented 4 years ago

Changes proposed in this pull request:

codecov-io commented 4 years ago

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%) :arrow_down:
Impacted Files Coverage Δ
src/app/components/nav/footer/footer.component.ts 57.57% <0%> (-6.07%) :arrow_down:

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 commented 4 years ago

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:

out

jayaike commented 4 years ago

I have removed the extra footer

jayaike commented 4 years ago

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

pushkalkatara commented 4 years ago

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

lunayach commented 4 years ago

@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, 12 and, 11

jayaike commented 4 years ago

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

jayaike commented 4 years ago

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

jayaike commented 4 years ago

Should I use the URL as a test?

jayaike commented 4 years ago

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 commented 4 years ago

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

jayaike commented 4 years ago

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 commented 4 years ago

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

lunayach commented 4 years ago

@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 commented 4 years ago

@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!! :smile:

jayaike commented 4 years ago

@lunayach , Alright not a problem. Thanks