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

Style ecr-library sort #2991

Merged
merged 13 commits into from
Dec 5, 2024
Merged

Style ecr-library sort #2991

merged 13 commits into from
Dec 5, 2024

Conversation

lina-roth
Copy link
Collaborator

PULL REQUEST

Screenshot 2024-12-05 at 10 35 08 AM

Summary

  • Remove outline on arrow
  • Remove highlight color
  • Move arrows to the far right ( sometimes appears as if not at the end because of the changing width of the rckms rule summary column)

Related Issue

Fixes #2860

Acceptance Criteria

  • Move sort arrows to the far right of the columns
  • The default sort is that the table is sorted by Received Date, with the most recent eCRs at the top. The arrow next to Received Date should be pointing up by default always to indicate this sort, but the column should not be highlighted blue until somebody clicks the arrow again. Received date is already the default sort, just turn off highlighting
  • Change highlighted column color to #DFE1E2. Check with design
  • Fix outline around SortButton when clicked to be a more proper size

Checklist

  • If this code affects the other scrum team, have they been notified? (In Slack, as reviewers, etc.)

@ashton-skylight
Copy link
Collaborator

Reviewed! Lina and I chatted about moving the filtering arrow to the far right for received date and encounter date. As the ticket calls out, due to the dynamic nature of the RCKMS summary, we had two options: (1) remove the responsive columns from RCKMS or (2) push the filtering arrows to the far right. We chose to keep the responsive columns in favor of user readability for the RCKMS summary.

Copy link
Collaborator

@mcmcgrath13 mcmcgrath13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing the highlight on the arrows is an accessibility issue. If you keyboard navigate through the viewer there is now no visual indication you are on a sort button that is interactive (Just kinda goes into the void until you end up on the names in the list)

@mcmcgrath13
Copy link
Collaborator

I think you can get the no outline for pointer users, but still outline with keyboard navigators by re-adding the styles with the :focus-visible selector

button:focus-visible {
  outline: 5px auto -webkit-focus-ring-color;
}

@ashton-skylight
Copy link
Collaborator

@mcmcgrath13 @lina-roth Great callout, thank you, Mary! Looking at the component now. The default state of the table with sorting does not highlight the column. If we keep that as the default state that should work, and then if the user clicks on the column, the header could change to base-lighter, #A9AEB1.

Screenshot 2024-12-05 at 11 57 08 AM

@lina-roth lina-roth requested a review from mcmcgrath13 December 5, 2024 17:34
@mcmcgrath13
Copy link
Collaborator

when encounter date is sorted, the arrow on patient name is cut off
image

@lina-roth
Copy link
Collaborator Author

lina-roth commented Dec 5, 2024

@mcmcgrath13 arrow isnt cutoff for me when sorting by encounter date
Screenshot 2024-12-05 at 12 51 41 PM

@lina-roth lina-roth requested a review from mcmcgrath13 December 5, 2024 17:54
@mcmcgrath13
Copy link
Collaborator

@lina-roth I think the difference has to do with what the rem is set to. I use a smaller font than standard (14), so I think that's what's causing the difference. We should probably account for that. I don't see the icon cut off on the main branch.

&.usa-button{
justify-content: flex-end;
margin-left: auto;
padding: .65rem .65rem .65rem .65rem;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since these are fit to an icon that is sized in px - we should probably use px here instead of rem. Inspector says the icons are 24x24

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@lina-roth lina-roth requested a review from mcmcgrath13 December 5, 2024 18:38
Copy link
Collaborator

@mcmcgrath13 mcmcgrath13 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (assuming acceptance of 12px change)

thank you!!

Co-authored-by: Mary McGrath <m.c.mcgrath13@gmail.com>
@lina-roth lina-roth enabled auto-merge December 5, 2024 19:02
@lina-roth lina-roth added this pull request to the merge queue Dec 5, 2024
Merged via the queue into main with commit 56997de Dec 5, 2024
15 checks passed
@lina-roth lina-roth deleted the lr/sort-style branch December 5, 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.

Styling sorting function of ECR Library
3 participants