kblincoe / VisualGit_SE701_2019_4

1 stars 0 forks source link

[200] Fixed reload and added to menu bar with a shortcut #228

Closed qhen143 closed 5 years ago

qhen143 commented 5 years ago

Resolves #200 . Followup to #216 .

After a bit of research, it turns out Angular 2 uses HTML5 style routing; pushstate routing by default. In that case, routing is handled from the client's side by changing the URL programmatically after making a request to the base page. Reloading a page causes issues because it makes a request to that modified URL instead of the base page. eg. Request for localhost:8080/ returns base page. Routing to localhost:8080/abc/420 is done client side without requests. However, when reloading, request is made for localhost:8080/abc/420 which it can't handle.

Workaround is to use Hash-based routing instead. In hash-based routing, data is prepended by a hash. However, when requesting a page, everything after the hash is ignored thus it makes a request to the same page. eg. localhost:8080/# and localhost:8080#/abc/123/ make requests to localhost:8080/, allowing for reloads.

Changes:

To test:

  1. Navigate to any page.
  2. Press Ctrl+R for windows or Command+R for Mac. Expected: Page should reload and redirect to the login page.
  3. Repeat steps 1-2 but in step 2 use: Reload from menu bar instead.

Due to lingering issues with #238, test reloading from the branch page.

  1. Navigate to the branch page.
  2. Reload using any method. Expected: Page should reload and redirect to login page.

Requesting Mac users to test.

r4inee commented 5 years ago

Works fine on Windows, good job.

@sloushsu the problem you met on Mac does not occur on my windows pc. Sign in -> open a local repo -> reload -> and the user is signed out and the page is reloaded (on windows). Is that the step you tested?

Yes, that's how I tested it. If you reload while on the graph panel, it will return you to the main panel but it throws the error shown above. Also, if reloading triggers a logout, it would make more sense for the application to transition back to the login screen.

qhen143 commented 5 years ago

@sloushsu Could not reproduce that error on windows. Aside from that, navigating back to the login screen would make sense with this implementation.

Main intent of this change was to fix the issue where the reload causes the application to hang due to invalid routes.

r4inee commented 5 years ago

@sloushsu Could not reproduce that error on windows. Aside from that, navigating back to the login screen would make sense with this implementation.

Main intent of this change was to fix the issue where the reload causes the application to hang due to invalid routes.

Are you able to add routing to login screen before this gets merged? Shouldn't be a big change and prevents opening additional issues.

r4inee commented 5 years ago

Hmm, still shows the error message but this can be addressed later (seems like other issues are experiencing path issues with different OSes as well). Routing back to login works nicely! Great work! 👊

qhen143 commented 5 years ago

@sloushsu @dylanfu Reloading now routes to Login page.

When reloading, app.component is re-initialised calling ngOnInit() to redirect back to login page. There were issues with the router navigation because app.component is initialised before the router navigation could finish. The workaround was to use a small timeout to let it finish. In later versions of router, we could just disable initialNavigation instead. Unfortunately, our router does not seem to support it.

Also, Ctrl+R does not call the same command as the reload from the menu. If there future plans to add additional features to the menu bar reload, Ctrl+R will not reflect this chnage, so further additions should be added to ngOnInit() instead.

I will update the testing above.

qhen143 commented 5 years ago

Also, removed dead subscriptions lingering from #238 .