-
Notifications
You must be signed in to change notification settings - Fork 18
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
#2763: Expanded row functionality - [DK] #2945
Conversation
…rtfolio_members_json response
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
1 similar comment
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
🥳 Successfully deployed to developer sandbox dk. |
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.
src/registrar/assets/js/get-gov.js
Outdated
try { | ||
if (!isNaN(parsedDate.getTime())) { // Check if the date is valid | ||
last_active_display = parsedDate.toLocaleDateString('en-US', options); | ||
last_active_sort_value = parsedDate.getTime(); // sort as numeric value, seconds since 1970 |
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.
(Q) Why are we sorting as a numeric value rather than on date? Totally fine with it, but doesn't USWDS have support for sorting dates?
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.
We have to sort dates as well as string values, namely 'Invalid date' and 'Invited'. Assigned each of these a numeric value (0 and 1) and sorted dates according to time since epoch
🥳 Successfully deployed to developer sandbox dk. |
Ah I see what is going on. Okay - I thought the domain assignment list was specific to the portfolio, i.e. it would only show the domains the user is assigned to given it. That makes sense as to why it would error out in this case. I'm assuming that this is desired right? |
@zander, noting here that you are correct. The desired behavior is to only include the domains which are part of the portfolio and are assigned to the user/email. I have fixed in the most recent push |
🥳 Successfully deployed to developer sandbox dk. |
let display_value = invalid_date; // Default display value for invalid or null dates | ||
let sort_value = -1; // Default sort value for invalid or null dates | ||
|
||
if (last_active === invited) { |
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.
Much better, nice work
* Behavior: | ||
* - The function checks the user's permissions (`member_permissions`) and generates | ||
* corresponding HTML sections based on the permission choices defined in `UserPortfolioPermissionChoices`. | ||
* - Permissions are categorized into domains, requests, and members: | ||
* - Domains: Determines whether the user can view or manage all or assigned domains. | ||
* - Requests: Differentiates between users who can edit requests, view all requests, or have no request privileges. | ||
* - Members: Distinguishes between members who can manage or only view other members. | ||
* - If no relevant permissions are found, the function returns a message stating that the user has no additional permissions. | ||
* - The resulting HTML always includes a header "Additional permissions for this member" and appends the relevant permission descriptions. | ||
*/ |
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.
🤌
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. Confirmed that the bug was no longer present. Really wonderful PR all around
Ticket
Resolves #2763
Changes
Context for reviewers
Setup
Code Review Verification Steps
As the original developer, I have
Satisfied acceptance criteria and met development standards
Ensured code standards are met (Original Developer)
Validated user-facing changes (if applicable)
As a code reviewer, I have
Reviewed, tested, and left feedback about the changes
Ensured code standards are met (Code reviewer)
Validated user-facing changes as a developer
New pages have been added to .pa11yci file so that they will be tested with our automated accessibility testing
Checked keyboard navigability
Meets all designs and user flows provided by design/product
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Tested with multiple browsers, the suggestion is to use ones that the developer didn't (check off which ones were used)
(Rarely needed) Tested as both an analyst and applicant user
Note: Multiple code reviewers can share the checklists above, a second reviewers should not make a duplicate checklist
As a designer reviewer, I have
Verified that the changes match the design intention
Validated user-facing changes as a designer
Checked keyboard navigability
Tested general usability, landmarks, page header structure, and links with a screen reader (such as Voiceover or ANDI)
Tested with multiple browsers (check off which ones were used)
(Rarely needed) Tested as both an analyst and applicant user
Screenshots