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

Functional users list overlays #483

Merged
merged 10 commits into from
Sep 13, 2024
Merged

Functional users list overlays #483

merged 10 commits into from
Sep 13, 2024

Conversation

echappen
Copy link
Contributor

@echappen echappen commented Sep 9, 2024

Blocked by PR #476

Changes proposed in this pull request:

  • Converts roles functionality in users list to overlays

Per engineering sync discussion, some additional acceptance criteria:

  • When opening the dialog, we'll keep focus on the close button and allow users to tab through the rest of the content.
  • Opening the dialog should also announce a title specific to the functionality being presented in the dialog.
  • When closing the dialog, we'll keep the default behavior of returning focus to the last point of regard.
  • When closing the dialog, we'll have SR's announce any success messages (while keeping default focus).
  • While dialog is open, we agreed that a user should be able to click outside of it to close it. Since this is not default behavior for dialogs, we're going to spike on this and see what we come up with.

How to test

  1. Log into the CloudFoundry dev environment through the CLI: cf login -a [url] --sso
  2. Start up a dev server with a refreshed CF token: npm run dev-cf
  3. Navigate to the users list: /orgs/[orgId]
  4. For the org roles form, click any link under the "organization roles" column
  5. For the space roles form, click any link under the "access permissions" column

Forms should operate the same as when they were on pages, with the only difference being that the overlay closes on form submission success.

Related issues

Closes #468

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

None, UI changes only

@echappen echappen mentioned this pull request Sep 10, 2024
2 tasks
@echappen echappen marked this pull request as ready for review September 11, 2024 16:18
@echappen echappen requested a review from a team as a code owner September 11, 2024 16:18
@echappen echappen changed the title WIP: Functional users list overlays Functional users list overlays Sep 11, 2024
@echappen
Copy link
Contributor Author

@beepdotgov @cannandev @hursey013 The only criteria I'm still having trouble with is announcing the success message—can't get it to announce. I'll keep working on that, but otherwise, I think this is ready for eyes!

@echappen
Copy link
Contributor Author

I'm going to make dynamic url params a separate GH issue—I might get to it depending on how much I need to update with this PR, but I don't want it to block the rest of the work.

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.

@echappen Seriously, this is looking fantastic so far. I’ve left a few design suggestions/requests, and a couple non-blocking comments. Please let me know if anything’s unclear, or if you’d like to discuss anything further!

@echappen
Copy link
Contributor Author

I’ve been working on getting aria-live regions to be more consistently read by VoiceOver. The lack of consistency still perplexes me.

I know that in order for aria-live regions to be read, two things need to be true (this article is a good summary):

  1. aria-live regions need to be present in the DOM on initial page load, and
  2. Screen readers will only read updates to these regions (it won’t read text already present on page load)

I suspect the inconsistency is coming down to the timing between a react component re-rendering (thus re-loading that part of the DOM) and a message updating. I think the reason that the success message wasn’t being read was because these things were happening at the same time.

Putting a 1/2 second delay on the message update solved this issue. But I’ve also been looking into alternatives, and I’m intrigued by this approach, which is now an npm package. This approach uses the React Context API, which I haven’t used at all, so I’m less comfortable with it. @cannandev @hursey013 Any thoughts on this approach and if we should try it?

@echappen echappen merged commit c70cf7b into main Sep 13, 2024
3 checks passed
@echappen echappen deleted the eoc/468-data branch September 13, 2024 19:11
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.

Build redesigned permission screens
3 participants