starter.dev-github-showcases icon indicating copy to clipboard operation
starter.dev-github-showcases copied to clipboard

[Angular-NgRx-SCSS] Audit app for any architectural improvements and features that are incomplete

Open Atrophius opened this issue 3 years ago • 3 comments

Acceptance

As either a write up in Google Docs or as comments on this issue, please provide us with:

  • Any bugs you find clicking through the app
  • Any architectural issues you see with components, their styles, or general cleanup required

Notes

See #489 for a good example

Atrophius avatar Sep 12 '22 13:09 Atrophius

Project setup

  • README doesn't mention having to run build:libs before the app is ready to run. Ideally it should be included as a prestart script

App issues

  • [ ] the main page doesn't have any loading indicator (like loading skeleton on angular-apollo-tailwind)
  • [ ] general note - there are no loading indicators across the whole app
  • [ ] same as https://github.com/thisdot/starter.dev-github-showcases/issues/489#issuecomment-1192909847
    • on http://localhost:4200/tvanantwerp page the link to the website is treated as a local link rather than a global link
  • On the repo page (eg. http://localhost:4200/thisdot/starter.dev):
    • [ ] issues tab doesn't show proper data
0 repo-issues-tab-broken
  • [ ] refreshing the issues/pull requests page breaks the page navigation
    • clicking on a different tab navigates to eg. http://localhost:4200/issues instead of http://localhost:4200/thisdot/starter.dev/issues
      • note: it works properly before refreshing
  • [ ] routing paths should follow the same convention as original GH eg. http://localhost:4200/thisdot/starter.dev/pull-requests should be http://localhost:4200/thisdot/starter.dev/pulls
0 repo-pr-wrong-url
  • [ ] navigation on pull requests page doesn't work (no new pages are shown) - couldn't test issues as the whole tab is not working atm
0 repo-prs-navigation-broken
  • [ ] in tree view when navigating the tree the root README is still shown.
    • [ ] It should not be shown if a given directory doesn't contain a readme file
    • [ ] if a subdirectory contains a README file then this one should be shown instead of the root one
0 repo-readme-should-not-be-shown
  • [ ] label and sort dropdowns don't show any options to choose
0 repo-pr-filters2 0 repo-prs-filters1
  • [ ] clicking on the x on the label and sort dropdowns doesn't close the dropdown (but it should)
  • [ ] clicking on the Readme button in the about section should scroll the page to the README section
0 repo-readme-link

ktrz avatar Sep 30 '22 16:09 ktrz

Code improvement ideas

  • [ ] app state for the most part doesn't need to be globally defined. Each sub-feature should be able to load necessary data for a given sub-page eg. repository should probably be added to a state via RepositoryModule
    • [ ] in some cases an app level state is beneficial as it is probably a good idea to load the authenticated user from the top of the app
  • [ ] IMO we should split profile and authenticated user into separate state slices. Authenticated user should be available always and currently when refreshing the profile page it is not loaded
  • [ ] the root src folder contains a mix of feature and technical directories. IMO it would be better to keep the root level directory structure focused on features only and split into technical directories ie. state, services, components further down the structure
    • probably the routing can be a source of truth for how we split the feature directories
    • any shared components can be (optionally) extracted into a library, but we can also think of shared as the same level as any other feature
  • [ ]the routing structure will probably benefit from a bit of restructuring.
    • my suggested structure
/ root - layout component (top nav + content below) - load authenticated user
  - /:owner (user module) - load profile
    - /:repo (repository module) - load repo
      - /issues (repo issues component) - load issues
      - /pulls (repo pulls component) - load pulls
      - /code (redirect to ./)
      - / (repo code component) - load tree structure - NOTE: use full path matching in the router configuration
      - / (file explorer module) - I think if we set it as prefix patch matching it should work nicely
        - /tree/:path 
        - /blob/:path
    - / (profile component) - NOTE: use full path matching in the router configuration
  - / (top repositories module) - NOTE: use full path matching in the router configuration

ktrz avatar Sep 30 '22 18:09 ktrz

Did not get all of these created into new issues, but did make some that were directly related to getting the main app functionality working: #833 , #834, #835.

lindakatcodes avatar Nov 04 '22 23:11 lindakatcodes