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

Refactoring App Module and App Routing Module #299

Closed
wants to merge 3 commits into from
Closed

Refactoring App Module and App Routing Module #299

wants to merge 3 commits into from

Conversation

Kajol-Kumari
Copy link
Member

@Kajol-Kumari Kajol-Kumari commented Mar 14, 2020 •
edited
Loading

This PR includes the implementation of one of the part of ideas proposed in here.

Features added:

  1. app-module splitted into smaller sub-module
  2. app-routing-module splitted into smaller sub-routes.

Refactoring Done for following Modules:

  • Auth Module
  • Challenge Module
  • Dashboard Module
  • Home Module
  • Nav Module
  • Publiclist Module
  • Utility Module

Refactoring TODO list:

  • Analysis Module
  • About Module
  • challenge-create Module
  • Contact Module
  • Get-involved Module
  • Not-found Module
  • Our-team Module
  • Privacy-Policy Module
  • Profile Module

GIF showing that all routes are working fine are refactoring:
ezgif com-video-to-gif (1)

Link to live demo: http://pr-299-evalai.surge.sh

Shekharrajak
Shekharrajak reviewed Mar 14, 2020
View reviewed changes
src/app/components/home/home.module.ts
RulesComponent,
TestimonialsComponent,
TwitterFeedComponent,
HomeComponent
Copy link
Member

Choose a reason for hiding this comment

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

HomeComponent uses the headers and footer component, which is not declared in this module. May be we can have NavModule (it will declare the header and footer that will be used across other module as well) which will be imported here.

WDYT ?

Copy link
Member Author

Choose a reason for hiding this comment

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

I had tried that too, but adding the NavModule is making the side-nav and top-nav appear but their routes are not working.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Shekharrajak I was missing to add the "RouterModule" in the custom modules due to which routerLinks were not getting activated. It's working fine now. I have comitted the required changes. Please look at it once and let me know if any modification is required.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please share the demo link, that surge create during travis CI/CD build ? You can also create some GIF of the working for future reference along with PR description.

src/app/components/home/home.module.ts
TwitterFeedComponent,
HomeComponent
],
schemas: [ CUSTOM_ELEMENTS_SCHEMA ],
Copy link
Member

Choose a reason for hiding this comment

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

What does it do ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Basically, it defines a schema that will allow any non-Angular elements with 'a -' in their name. Without this, all the selector tags were throwing error of being undefined.

@Shekharrajak
Copy link
Member

Please try to make better PR title and commit message.

@Kajol-Kumari Kajol-Kumari changed the title using_feature_modules_for_app_module_and_app_routing_module Refactoring App Module and App Routing Module Mar 14, 2020
@codecov-io
Copy link

Codecov Report

Merging #299 into master will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #299      +/-   ##
==========================================
+ Coverage   50.62%   50.64%   +0.02%     
==========================================
  Files          66       66              
  Lines        3771     3771              
  Branches      444      444              
==========================================
+ Hits         1909     1910       +1     
+ Misses       1767     1766       -1     
  Partials       95       95
Impacted Files Coverage Δ
...bliclists/challengelist/challengelist.component.ts 47.57% <0%> (+0.97%) ⬆️
Impacted Files Coverage Δ
...bliclists/challengelist/challengelist.component.ts 47.57% <0%> (+0.97%) ⬆️

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 d3d23ca...3eba3d0. Read the comment docs.

@Kajol-Kumari Kajol-Kumari marked this pull request as ready for review March 14, 2020 10:07
Shekharrajak
Shekharrajak reviewed Mar 15, 2020
View reviewed changes
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.

Thanks for the work! Can you please create a list of modules that you created in this PR and TODO modules that is not cover in this PR? You can create a checklist in PR description or somewhere in different newly opened issue.

src/app/components/home/home.module.ts
RulesComponent,
TestimonialsComponent,
TwitterFeedComponent,
HomeComponent
Copy link
Member

Choose a reason for hiding this comment

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

Can you please share the demo link, that surge create during travis CI/CD build ? You can also create some GIF of the working for future reference along with PR description.

Shekharrajak
Shekharrajak approved these changes Mar 16, 2020
View reviewed changes
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.

Thanks for GIF and PR description update. May be you can update it in idea list task for future reference. @Sanji515 can you please check it locally once and approve , if it is looking fine ?

@RishabhJain2018
Copy link
Member

@Sanji515 Can you please take a look at this?
@Kajol-Kumari Can you please create an issue for the remaining work and you can add it in a new PR.

@Kajol-Kumari Kajol-Kumari mentioned this pull request May 20, 2020
Open
9 tasks
@codecov-commenter
Copy link

Codecov Report

Merging #299 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #299   +/-   ##
=======================================
  Coverage   50.63%   50.63%           
=======================================
  Files          66       66           
  Lines        3772     3772           
  Branches      444      444           
=======================================
  Hits         1910     1910           
  Misses       1767     1767           
  Partials       95       95

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 e5816b9...975444a. Read the comment docs.

Sanji515
Sanji515 approved these changes May 20, 2020
View reviewed changes
Copy link
Member

@Sanji515 Sanji515 left a comment

Choose a reason for hiding this comment

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

Good Work @Kajol-Kumari
LGTM 👍 This can be merged 🎉

@Kajol-Kumari Kajol-Kumari mentioned this pull request May 20, 2020
Merged
11 tasks
lunayach
lunayach approved these changes May 21, 2020
View reviewed changes
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.

Looks good to me, LGTM 🎉

@Sanji515
Copy link
Member

@Kajol-Kumari I think we can close this PR as the work done in this PR is already included in #303

@Kajol-Kumari
Copy link
Member Author

@Kajol-Kumari I think we can close this PR as the work done in this PR is already included in #303

yes @Sanji515 agree with you.will close this one.

@Kajol-Kumari Kajol-Kumari deleted the separate_app_routes_into_sub_routes branch May 31, 2020 06:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Sanji515 Sanji515 Sanji515 approved these changes

@lunayach lunayach lunayach approved these changes

@Shekharrajak Shekharrajak Shekharrajak approved these changes

Assignees
No one assigned
Labels
can-be-merged
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Kajol-Kumari @Shekharrajak @codecov-io @RishabhJain2018 @codecov-commenter @Sanji515 @lunayach