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

feat: UI overhaul #119

Merged
merged 71 commits into from
May 24, 2024
Merged

feat: UI overhaul #119

merged 71 commits into from
May 24, 2024

Conversation

sutterj
Copy link
Contributor

@sutterj sutterj commented May 16, 2024

Pull Request

Proposed Changes

  • Closes github/ospo#2242
    • Replace ICF pages router with app directory
      • This change comes as a refactor and moves the React components from /src/pages to /src/app
    • Create a standard page layout
      • The app router uses layout files that are inherited by child components
      • The header (for example) is at the highest level layout file (/src/app/layout.tsx) to appear on all pages
    • Customize NextAuth login page
      • Middleware updated to enforce login (/src/middleware.ts)
      • Custom auth pages added for login and error (/src/app/auth)
      • Specify custom pages (/src/app/api/auth/lib/nextauth-options.ts)
    • Fix issue with authentication of JWT being longer than expiration of github token
      • Check current time against token expiration (/src/app/[organizationId]/layout.tsx)
    • Implement new designs for the rest of the pages and flows
      • This was the bulk of the changes with new files/dirs being created for components (contained in /src/app)

Readiness Checklist

Author/Contributor

  • If documentation is needed for this change, has that been included in this pull request
  • run npm run lint and fix any linting issues that have been introduced
  • run npm run test and run tests

Reviewer

  • Label as either bug, documentation, enhancement, infrastructure, maintenance, or breaking

sutterj added 30 commits April 23, 2024 13:29
- Remove parent orgs route since we only have one org
- Remove orgs page as well
- refactor project to use the newer app router
- rework nextauth and trpc for new router
- add header with org for all child pages
- move reused hooks into functions
- remove config as bodyParser is not needed with the app router
- Errors were caused by probot middleware being in the app router api
- Moved webhooks back to pages router and disabled auth middleware for webhooks
- Add query to edit repo name
- Update packages to latest (except octokit)
- Add and move dialogs to folders
- Add and move flashes to folders
- Add breadcrumbs
- There should be a way to do this with jest/ts-jest config
- Not working so reverting for now
- Currently error is null
- Ideally would get error message back to include in flash
@ahpook
Copy link
Contributor

ahpook commented May 17, 2024

Operations I've tried:

  • login ✅
  • filter forks in 'find a fork' box ✅
  • follow pagination breadcrumbs forward and back, both numbers and previous/next nav ✅
  • follow a link to a fork ✅
  • use the search bar on the mirror page to filter to a subset of mirrors ✅
  • use the ... context - edit - rename a mirror ✅
  • use the + Create mirror to create a new mirror ✅
  • use the ... context - delete mirror 🗑️ to remove it ✅

things i noticed

  • the org name at the header takes me to the organization page, from a repo's mirrors page i sort of expected it to navigate back up to the list of all forks
  • the pagination at 5 seems pretty small, maybe the default could be increased to 10 or the pagination value selectable
  • Need to think about showing the discrepancy between the number of branches listed on the All forks page and the mirrors page - it says "17" for metrics but only 3 mirrors, what are the others? (Related to Simplify/rethink branch->mirror relationship #114)

Overall this is super awesome work and a huuuuuuge uplift for the app overall!!

@ahpook
Copy link
Contributor

ahpook commented May 17, 2024

(none of my comments above should be considered a blocker for merging this BTW, all follow-up ideas)

@ahpook
Copy link
Contributor

ahpook commented May 24, 2024

Couple nits that I will make follow-up issues for but I ran through the app and all the functionality works and looks a m a z i n g 👍 :shipit: 🚀

@ajhenry ajhenry merged commit d16bacd into main May 24, 2024
13 checks passed
@sutterj sutterj deleted the sutterj/ui-overhaul branch May 31, 2024 18:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants