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

Implements all orgs page #548

Merged
merged 23 commits into from
Oct 18, 2024
Merged

Implements all orgs page #548

merged 23 commits into from
Oct 18, 2024

Conversation

echappen
Copy link
Contributor

@echappen echappen commented Oct 4, 2024

Changes proposed in this pull request:

  • Implements UI design for "all orgs" page
  • Adds a cookie to save last visited org id; sets link to that org on "all orgs" page

Notes

  • @beepdotgov I assumed we'd want commas for large memory usage numbers, but it does make them a bit wider.

Pending

  • Tests for LastViewedOrgLink are failing because cookies can't be mocked there for some reason?? Weird.

Questions

  • Should we set an expiration for the lastViewedOrgId cookie?

Related issues

Closes #539

How to test

  1. Log into CF via the command line, then run npm run dev-cf
  2. Visit /orgs

Screenshots

Desktop:

Screenshot 2024-10-09 at 12 15 42 PM

Mobile:

Screenshot 2024-10-07 at 4 05 20 PM

Submitter checklist

  • Added logging is not capturing sensitive data and is set to an appropriate level (DEBUG vs INFO etc)
  • Updated relevant documentation (README, ADRs, explainers, diagrams)

Security considerations

Pulls various data from CAPI to show page

@echappen echappen changed the title WIP: implements all orgs page implements all orgs page Oct 7, 2024
@echappen echappen changed the title implements all orgs page Implements all orgs page Oct 7, 2024
@echappen echappen marked this pull request as ready for review October 7, 2024 21:11
@echappen echappen requested a review from a team as a code owner October 7, 2024 21:11
@echappen
Copy link
Contributor Author

echappen commented Oct 7, 2024

@beepdotgov @cannandev I still need to collect the real org roles, but there's enough here for a first round of review.

Copy link
Contributor

@beepdotgov beepdotgov left a 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.

src/components/MemoryBar.tsx Outdated Show resolved Hide resolved
src/components/OrganizationsList/OrganizationsList.tsx Outdated Show resolved Hide resolved
src/components/MemoryBar.tsx Show resolved Hide resolved
@echappen
Copy link
Contributor Author

echappen commented Oct 9, 2024

@beepdotgov @hursey013 Real org roles are now in! To see them locally, you'll need to add a CF_USER_ID environment variable to .env.local and set it to a real user guid. You can get your own user ID by logging into CF through the command line, then running cf curl /v3/users and looking at the guid for your username. (When deployed, the user ID will come from UAA/authentication.)

@echappen
Copy link
Contributor Author

echappen commented Oct 9, 2024

@beepdotgov @hursey013 For the "at a glance" section: currently, we have a page for users (/orgs/:id) and a functional page for applications (/orgs/:id/apps). We don't have a page for spaces yet—the closest page we have for that is the users one. For now, should I:

  1. link to /orgs/:id for spaces,
  2. link to a placeholder page for spaces, or
  3. remove the link entirely?

@beepdotgov
Copy link
Contributor

We don't have a page for spaces yet—the closest page we have for that is the users one. For now, should I:

  1. link to /orgs/:id for spaces,
  2. link to a placeholder page for spaces, or
  3. remove the link entirely?

I’d vote 3: remove the link for now. @hursey013?

@beepdotgov
Copy link
Contributor

Real org roles are now in! To see them locally, you'll need to add a CF_USER_ID environment variable to .env.local and set it to a real user guid.

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?

image

(Other pages load just fine.)

@echappen
Copy link
Contributor Author

echappen commented Oct 10, 2024

is that because I haven’t set the user id yet?

@beepdotgov I think so—try setting it and if the issue persists, lmk! (I can DM you my ID if that's easier)

@beepdotgov
Copy link
Contributor

@echappen I’m in, thanks for the help!

@echappen
Copy link
Contributor Author

@beepdotgov I put a last updated timestamp on the bottom of the page, welcome any design thoughts you may have:

Screenshot 2024-10-10 at 1 55 53 PM

@beepdotgov
Copy link
Contributor

@echappen That looks fantastic! I’d suggest two changes, let me know what you think:

  • I’d recommend we pull it up into the page intro, so the information’s more readily available. (See attached very quick mockup.)
  • I’d also suggest adding a bit more detail to the timestamp? Maybe add in year and timezone to be super explicit?
View organizations

What do you think, though?

@echappen
Copy link
Contributor Author

@beepdotgov Sounds good, will do!

@echappen
Copy link
Contributor Author

@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.

Screenshot 2024-10-15 at 3 36 52 PM

@beepdotgov
Copy link
Contributor

@echappen This looks AMAZING, fantastic work! 🎉♾️🎉

@echappen echappen requested a review from beepdotgov October 16, 2024 14:23
beepdotgov
beepdotgov previously approved these changes Oct 16, 2024
Copy link
Contributor

@beepdotgov beepdotgov left a 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})/
Copy link
Contributor

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?

Copy link
Contributor Author

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.

.env.example.local Outdated Show resolved Hide resolved
Copy link
Contributor

@beepdotgov beepdotgov left a 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 )

@echappen echappen merged commit e60e8c4 into main Oct 18, 2024
3 checks passed
@echappen echappen deleted the eoc/539 branch October 18, 2024 16:24
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

Successfully merging this pull request may close these issues.

Implement "all organizations" page
3 participants