-
Notifications
You must be signed in to change notification settings - Fork 0
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
Implements all orgs page #548
Conversation
@beepdotgov @cannandev I still need to collect the real org roles, but there's enough here for a first round of review. |
Only thing left to do is roles
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eleni, this looks stellar. I left a few non-blocking notes, and made a couple design requests. But seriously, this looks really, really great.
@beepdotgov @hursey013 Real org roles are now in! To see them locally, you'll need to add a |
@beepdotgov @hursey013 For the "at a glance" section: currently, we have a page for users (
|
I’d vote 3: remove the link for now. @hursey013? |
Exciting, thanks @echappen! I haven’t logged in via the command line yet, but I’ll sort that out tomorrow. I am seeing a runtime error when I try to view /orgs locally; is that because I haven’t set the user id yet? (Other pages load just fine.) |
@beepdotgov I think so—try setting it and if the issue persists, lmk! (I can DM you my ID if that's easier) |
@echappen I’m in, thanks for the help! |
@beepdotgov I put a last updated timestamp on the bottom of the page, welcome any design thoughts you may have: |
@echappen That looks fantastic! I’d suggest two changes, let me know what you think:
What do you think, though? |
@beepdotgov Sounds good, will do! |
@beepdotgov Here's a screen of the infinity design in action (the dev data doesn't return an org with unlimited memory allocation). I'm having screen readers ignore the infinity symbol entirely since it's only decorative, given the text below. |
@echappen This looks AMAZING, fantastic work! 🎉♾️🎉 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This really looks remarkable, @echappen. Left two non-blocking comments, but those could absolutely be tackled in later issues.
Seriously, this is wonderful work. Thanks for lighting this up so beautifully.
|
||
export function setLastViewedOrg(request: NextRequest, response: NextResponse) { | ||
const matches = request.nextUrl.pathname.match( | ||
/orgs\/([0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-5][0-9a-fA-F]{3}-[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12})/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[question] Regex intimidates me - does this match a GUID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does! (Thanks, Stack Overflow). I need this GUID regex in a few places, so I tried to make it a variable, but I was having trouble inserting the variable into the larger regex pattern here. If you have any ideas there on how to make that more DRY, would love to hear them.
moves contextual styling/text into parent component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, works great, let’s ship it! 🚀
(As discussed in Slack, the “last accessed” organization isn’t getting displayed correctly, but that’s not a blocker for merge. We’ll tackle that in a separate issue: #565 )
Changes proposed in this pull request:
Notes
Pending
Questions
lastViewedOrgId
cookie?Related issues
Closes #539
How to test
npm run dev-cf
/orgs
Screenshots
Desktop:
Mobile:
Submitter checklist
Security considerations
Pulls various data from CAPI to show page