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.

Sign up for GitHub

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

On ws notifications, update name, details, and state in flow sidebar #1353

Merged
merged 5 commits into from
Nov 26, 2024
Merged

On ws notifications, update name, details, and state in flow sidebar #1353

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5b1f5b5
Update name in flow sidebar on ws notification model change
nmajor25 Nov 22, 2024
b66de1c
Update details in flow sidebar on ws notification model change
nmajor25 Nov 22, 2024
b5dc045
Update state in flow sidebar on ws notification model change
nmajor25 Nov 22, 2024
af91a01
Test that owner is updated in flow sidebar on ws notification model c…
nmajor25 Nov 22, 2024
8df0bf6
Refactor flow sidebar views
nmajor25 Nov 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/js/views/patients/sidebar/flow/flow-sidebar.hbs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<div>
<div class="patient-flow-sidebar__name">{{ name }}</div>
<div class="sidebar__details">{{ details }}{{#unless details}}<span class="sidebar--no-results">{{ @intl.patients.sidebar.flow.flowSidebarTemplate.noDetails }}</span>{{/unless}}</div>
<div data-name-region></div>
<div data-details-region></div>
Comment on lines +2 to +3
Copy link

Choose a reason for hiding this comment

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

💡 Codebase verification

Accessibility attributes need to be added to the new regions

The verification reveals that the new name and details regions lack proper accessibility attributes, while other regions in the codebase consistently use semantic HTML with proper labeling. To maintain accessibility:

  • Add aria-label or associate the regions with their respective heading labels using aria-labelledby
  • Ensure the dynamic content maintains a proper heading structure
  • Consider adding appropriate ARIA roles based on the content type

Example locations needing attention:

  • src/js/views/patients/sidebar/flow/flow-sidebar.hbs: Add accessibility attributes to data-name-region and data-details-region
  • Corresponding view files: Ensure rendered content follows accessibility patterns seen in other sidebar regions
🔗 Analysis chain

LGTM! Verify accessibility attributes.

The shift to region-based rendering is a good architectural choice that enables real-time updates and better separation of concerns. However, ensure that the new views maintain proper accessibility.

Let's verify the accessibility attributes in the corresponding view files:

Let me gather more information about the views to better understand the accessibility implementation.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check if the new views implement proper accessibility attributes

# Search for aria attributes in the related view files
rg -A 5 'NameView|DetailsView' | rg -i 'aria-|role'

# Check for any existing accessibility patterns in other sidebar regions
rg -A 5 'data-\w+-region' | rg -i 'aria-|role'

Length of output: 1545

Script:

#!/bin/bash
# Search for the NameView and DetailsView implementations
ast-grep --pattern 'class $NAME extends $_ {
  $$$
}'

# Search for any accessibility-related patterns in the flow sidebar views
rg -l "flow.*sidebar" | xargs rg -i "aria-|role|tabindex|label|description"

# Check template files for existing accessibility patterns
fd "\.hbs$" | xargs rg -i "aria-|role|tabindex|label|description"

Length of output: 17738

<div class="flex u-margin--t-16"><h4 class="sidebar__label u-margin--t-8">{{ @intl.patients.sidebar.flow.flowSidebarTemplate.stateLabel }}</h4><div class="flex-grow" data-state-region></div></div>
<div class="flex u-margin--t-8"><h4 class="sidebar__label u-margin--t-8">{{ @intl.patients.sidebar.flow.flowSidebarTemplate.ownerLabel }}</h4><div class="flex-grow" data-owner-region></div></div>
<div data-permission-region></div>
Expand Down
31 changes: 30 additions & 1 deletion src/js/views/patients/sidebar/flow/flow-sidebar_views.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,21 @@ const MenuView = View.extend({
},
});

const NameView = View.extend({
className: 'patient-flow-sidebar__name',
template: hbs`{{ name }}`,
});

const DetailsView = View.extend({
className: 'sidebar__details',
template: hbs`
{{ details }}
{{#unless details}}
<span class="sidebar--no-results">{{ @intl.patients.sidebar.flow.flowSidebarTemplate.noDetails }}</span>
{{/unless}}
`,
});

const PermissionView = View.extend({
className: 'flex u-margin--t-8',
template: hbs`
Expand All @@ -73,6 +88,8 @@ const PermissionView = View.extend({
const SidebarView = View.extend({
template: FlowSidebarTemplate,
regions: {
name: '[data-name-region]',
details: '[data-details-region]',
state: '[data-state-region]',
owner: '[data-owner-region]',
permission: '[data-permission-region]',
Expand All @@ -88,7 +105,9 @@ const SidebarView = View.extend({
};
},
modelEvents: {
'change:_state': 'showOwner',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure why this was showOwner? May be missing something, but I think showFlow is what this should be.

Copy link
Member

Choose a reason for hiding this comment

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

canEdit is only affected by the owner. I think this should just be 'change:_state': 'showState'

Copy link
Contributor Author

@nmajor25 nmajor25 Nov 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So this was originally showOwner because of this: https://github.com/RoundingWell/care-ops-frontend/blob/develop/src/js/views/patients/sidebar/flow/flow-sidebar_views.js#L133.

If the flow state is set to Done, the owner component becomes disabled.

So I have it set to 'change:_state': 'showActions'. Since the change:_state model events listener needs to now update the state component and potentially the owner component if state is set to done.

But in this scenario this.canEdit = this.model.canEdit(); and this.showPermission() would be ran for no purpose.

'change:name': 'showName',
'change:details': 'showDetails',
'change:_state': 'showFlow',
'change:_owner': 'showFlow',
},
onRender() {
Expand All @@ -98,13 +117,23 @@ const SidebarView = View.extend({
showFlow() {
this.canEdit = this.model.canEdit();

this.showName();
Copy link
Member

Choose a reason for hiding this comment

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

I don't think name or details need to be in this function.. they can just be in onRender.. this function appears to be needed if something changes that affects canEdit

Copy link
Member

Choose a reason for hiding this comment

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

really I think this method did more a bit ago and it likely doesn't make any sense now.. this.canEdit = this.model.canEdit(); should move into showActions and changing the owner should do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I moved showName() and showDetails() up to the render function. And moved the this.canEdit = ... to showActions().

this.showDetails();
this.showActions();
},
showActions() {
this.showState();
this.showOwner();
this.showPermission();
},
showName() {
const nameView = new NameView({ model: this.model });
this.showChildView('name', nameView);
},
showDetails() {
const detailsView = new DetailsView({ model: this.model });
this.showChildView('details', detailsView);
},
paulfalgout marked this conversation as resolved.
Show resolved Hide resolved
showState() {
if (!this.canEdit) {
const readOnlyStateView = new ReadOnlyStateView({ model: this.model });
Expand Down
21 changes: 16 additions & 5 deletions test/integration/patients/patient/patient-flow.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2421,18 +2421,19 @@ context('patient flow page', function() {
.find('.patient-flow__name')
.contains('New Flow Name');

cy
.get('.app-frame__sidebar')
.as('flowSidebar')
.find('[data-name-region]')
.should('contain', 'New Flow Name');

cy
.get('.app-frame__sidebar')
.find('.sidebar__footer')
.contains('Updated')
.next()
.should('contain', formatDate(testTs(), 'AT_TIME'));

cy
.get('.app-frame__sidebar')
.find('.js-close')
.click();

cy.sendWs({
category: 'DetailsChanged',
resource: {
Expand All @@ -2451,6 +2452,11 @@ context('patient flow page', function() {
.find('.patient-flow__details')
.contains('New flow details');

cy
.get('@flowSidebar')
.find('[data-details-region]')
.should('contain', 'New flow details');

cy.sendWs({
category: 'NameChanged',
resource: {
Expand Down Expand Up @@ -2604,5 +2610,10 @@ context('patient flow page', function() {
cy
.get('[data-header-region]')
.find('[data-state-region] .fa-circle-check');

cy
.get('@flowSidebar')
.find('[data-state-region]')
.should('contain', 'Done');
});
});