Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Atrophius opened this issue Sep 12, 2022 · 3 comments
Assignees

Comments

@Atrophius
Copy link

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

@ktrz
Copy link
Contributor

ktrz commented Sep 30, 2022

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

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
Copy link
Contributor

ktrz commented Sep 30, 2022

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

@lindakatcodes
Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants