[terra-clinical-item-view] Add property to separate by row or column when in two column layout #912

RayGunY · 2023-08-30T22:47:48Z

Summary

What was changed:

Added a trueColumn prop that defaults to true. When it is set to false it reverts to handling two column layout like it was before the changes in this PR. This means the screenreader will read out left to right, up to down. Also resizing/truncation/overflow will behave slightly differently (like it was working before) than trueColumn view because displays are being handled by row.
Added some styling changes to truncated content when it is in true two column view. There was a truncation issue when the ItemView itself didn't have isTruncated set to true but the ItemDisplays passed in did have isTruncated set. Added a check for the displays to see if isTruncated is set on them and if so then to use the truncated two column styling class. Added screenshots below to show the difference.
Added several tests for the above behavior of the different potential cases of left, right, or both having truncated displays.
I also pulled out some of the logic in the larger methods into their own smaller methods for readability.

Why it was changed:
So that users can have the option to use the old layout/handling for two column in case the new changes break anything for them. Also to fix truncation behavior for partial truncation.

Testing

This change was tested using:

Reviews

In addition to engineering reviews, this PR needs:

UX review
Accessibility review
Functional review

Additional Details

This PR resolves:

UXPLATFORM-9563

VO and JAWs Readouts:

VO-twoColumn-by-row.mov

JAWS-readout-by-row.mov

True two column resizing behavior is the same as before (truncated and not truncated):

truncation_resize.mov

non_truncated_resize.mov

This is the partial truncation issue I fixed:

Before

After

RayGunY · 2023-08-31T14:20:06Z

packages/terra-clinical-item-view/tests/wdio/item-view-spec.js

-    Terra.validates.element('with truncation in one column', { selector: '#ItemView-one-wrap' });
-    Terra.validates.element('with truncation in two columns', { selector: '#ItemView-two-wrap' });
+    Terra.validates.element('with truncation in one column', { selector: '#ItemView-one-truncate' });
+    Terra.validates.element('with truncation in two columns', { selector: '#ItemView-two-truncate' });


Noticed that previously this was not actually pointing to the tests for truncated content. So any of the 'with truncation in one column' or 'with truncation in two columns' snapshots are going to be changed for good reason.

Also some of the 'with word wrap' images have changed a bit as well - just the sizing of that image has changed but not the actual word wrap/overflow behavior. I will post recordings in the description to show this.

packages/terra-clinical-item-view/src/ItemView.jsx

...erra-clinical-item-view/src/terra-dev-site/doc/clinical-item-view/clinicalItemView.1.doc.mdx

packages/terra-clinical-item-view/src/ItemView.jsx

RayGunY · 2023-09-08T14:39:42Z

@eawww @sycombs @sdadn this is ready to be looked at when you have a chance

packages/terra-clinical-item-view/src/ItemView.jsx

sdadn · 2023-09-11T21:31:44Z

How does this affect existing behavior? Will this change be passive?

RayGunY · 2023-09-11T21:51:31Z

How does this affect existing behavior? Will this change be passive?

@sdadn This is essentially adding back old code to make sure we maintain an option for passivity. When this new prop is false it will revert to using the logic we had before the most recent two column accessibility updates.

RayGunY · 2023-09-14T18:24:53Z

How does this affect existing behavior? Will this change be passive?

@sdadn This is essentially adding back old code to make sure we maintain an option for passivity. When this new prop is false it will revert to using the logic we had before the most recent two column accessibility updates.

@sdadn tagging again, just want to make sure you saw my response

sdadn · 2023-09-21T22:14:53Z

packages/terra-clinical-item-view/src/ItemView.jsx

+      while (displaysSlice.length) {
+        displayGroups.push(displaysSlice.splice(0, 2));


Isn't this pushing the same array information to the array multiple times?

No, splice removes the first two displays from displaysSlice and pushes them together to displayGroups. So each time it's taking the top two off and pushing them. It's the same logic that was being used previously.

So displayGroups would look like [[display 1, display 2], [display 3, display 4], [display 5, display 6], [display 7, display 8]]. And by the end displaysSlice would be empty

Ah right, splice mutates the original array as opposed to slice. +1

sdadn · 2023-09-21T22:36:19Z

packages/terra-clinical-item-view/tests/wdio/item-view-spec.js

@@ -1,12 +1,19 @@
 Terra.describeViewports('Clinical Item View', ['tiny', 'small', 'medium', 'large', 'huge', 'enormous'], () => {
-  it('with one column displays', () => {
+  it('with displays', () => {


I know this is following the previous sentence structure, but since we are updating these, we should reword these so it reads as a sentence starting with "it". E.g. "It renders with with displays" or any other more appropriate descriptor.

sure thing, cf417ee

sdadn · 2023-09-21T22:40:52Z

@sdadn This is essentially adding back old code to make sure we maintain an option for passivity. When this new prop is false it will revert to using the logic we had before the most recent two column accessibility updates.

@RayGunY The old logic should be the default for passivity. A few consumers reported the recent updates causing issues. Though the question is if swapping the defaults breaks more or less consumers 😅.

RayGunY · 2023-09-22T13:41:34Z

@sdadn This is essentially adding back old code to make sure we maintain an option for passivity. When this new prop is false it will revert to using the logic we had before the most recent two column accessibility updates.

@RayGunY The old logic should be the default for passivity. A few consumers reported the recent updates causing issues. Though the question is if swapping the defaults breaks more or less consumers 😅.

I understand what you're saying Saad. @scott what do you think? I thought we wanted to push people towards using the new logic but have the option to keep the old logic for passivity.

mjpalazzo · 2023-09-22T14:03:59Z

@RayGunY - The conflict between passivity and accessibility is the challenge we are facing. Because consumers have not made their accessibility updates yet, I think we need to go with the passive approach and emphasize in the accessibility guide that consumers must enable the prop when they are remediating for accessibility. Other thoughts?

RayGunY · 2023-09-22T14:16:05Z

@RayGunY - The conflict between passivity and accessibility is the challenge we are facing. Because consumers have not made their accessibility updates yet, I think we need to go with the passive approach and emphasize in the accessibility guide that consumers must enable the prop when they are remediating for accessibility. Other thoughts?

@mjpalazzo that works for me, just wasn't sure which is the bigger priority in this case. I can go ahead and change the default to use the old logic.

RayGunY · 2023-09-22T17:26:21Z

@sdadn prop default changes have been made here: 456b749

sdadn · 2023-09-22T21:08:50Z

packages/terra-clinical-item-view/CHANGELOG.md

@@ -2,6 +2,9 @@

 ## Unreleased

+* Added
+  * Added `trueColumn` prop so consumers can choose whether to separate displays in the two column layout by row or by column.


Suggested change

* Added `trueColumn` prop so consumers can choose whether to separate displays in the two column layout by row or by column.

* Added `trueColumn` prop to toggle between displaying in a two column layout by row or by column.

Fixed: abe1fee#diff-019a0d835127f70f44b926383f7f9ca20fa8848183637b56a0fe47a22104b394R6

sdadn · 2023-09-22T21:09:19Z

packages/terra-clinical-item-view/src/ItemView.jsx

+  /**
+  * Determines whether the displays are programmatically separated by row or as true columns when layout is set to `twoColumns`.
+  * Screenreaders will read `trueColumn` displays from top to bottom, one column at a time.
+  * However, this prop defaults to false and therefore the screenreader will read the displays in both columns together, left to right, one row at a time.


Suggested change

* However, this prop defaults to false and therefore the screenreader will read the displays in both columns together, left to right, one row at a time.

* However, this prop defaults to `false` and therefore the screenreader will read the displays in both columns together, left to right, one row at a time.

abe1fee#diff-a09eb4a886b729c86a83daac740d9a14a92e66fe733a7a907899150b7fdae956R35

sdadn · 2023-09-22T21:09:26Z

packages/terra-clinical-item-view/src/ItemView.jsx

+  * Determines whether the displays are programmatically separated by row or as true columns when layout is set to `twoColumns`.
+  * Screenreaders will read `trueColumn` displays from top to bottom, one column at a time.
+  * However, this prop defaults to false and therefore the screenreader will read the displays in both columns together, left to right, one row at a time.
+  * For accessibility purposes it is recommended to set trueColumn to true.


Suggested change

* For accessibility purposes it is recommended to set trueColumn to true.

* For accessibility purposes it is recommended to set trueColumn to `true`.

abe1fee#diff-a09eb4a886b729c86a83daac740d9a14a92e66fe733a7a907899150b7fdae956R36

sdadn · 2023-09-22T21:11:49Z

packages/terra-clinical-item-view/src/ItemView.jsx

+      while (displaysSlice.length) {
+        displayGroups.push(displaysSlice.splice(0, 2));


Ah right, splice mutates the original array as opposed to slice. +1

sdadn · 2023-09-22T21:17:15Z

packages/terra-clinical-item-view/src/terra-dev-site/doc/example/ItemViewTwoColumnByRow.jsx

+import IconBriefcase from 'terra-icon/lib/icon/IconBriefcase';
+import IconPerson from 'terra-icon/lib/icon/IconPerson';


We can update these to use the new named exports introduced in cerner/terra-core#3915.

abe1fee#diff-8a154576b72d3f91339b6cfa473a6c31078e664975dd661f3f49febe659a5ba4R2

sdadn · 2023-09-22T21:23:04Z

We should probably have an example/code snippet where trueColumn=false and maybe compares it with trueColumn=true to highlight the differences.

RayGunY · 2023-09-25T13:43:31Z

We should probably have an example/code snippet where trueColumn=false and maybe compares it with trueColumn=true to highlight the differences.

@sdadn I believe I already have something that should cover that here: https://github.com/cerner/terra-clinical/pull/912/files#diff-d43cc843d972ecc662a6ef6a0bd194724cf3d97c03658812c7a0f855e209d881R41

The other two column examples on the page are all with trueColumn=true

sdadn · 2023-09-25T17:10:48Z

We should probably have an example/code snippet where trueColumn=false and maybe compares it with trueColumn=true to highlight the differences.

@sdadn I believe I already have something that should cover that here: https://github.com/cerner/terra-clinical/pull/912/files#diff-d43cc843d972ecc662a6ef6a0bd194724cf3d97c03658812c7a0f855e209d881R41

The other two column examples on the page are all with trueColumn=true

Ah I missed that. Looks good then!

RayGunY self-assigned this Aug 30, 2023

RayGunY added the 📦 terra-clinical-item-view label Aug 30, 2023

github-actions bot temporarily deployed to preview-pr-912 August 30, 2023 22:48 Destroyed

RayGunY changed the title ~~[clinical-item-view] Add row vs column view prop~~ [terra-clinical-item-view] Add property to separate by row or column when in two column layout Aug 30, 2023

github-actions bot temporarily deployed to preview-pr-912 August 30, 2023 22:57 Destroyed

RayGunY commented Aug 31, 2023

View reviewed changes

RayGunY requested review from sdadn, sycombs, chenyixuan625 and vinaybhargavar August 31, 2023 15:12

github-actions bot temporarily deployed to preview-pr-912 August 31, 2023 16:33 Destroyed

RayGunY force-pushed the UXPLATFORM-9563 branch from 2eb3fa5 to 1ac0d15 Compare August 31, 2023 16:55

github-actions bot temporarily deployed to preview-pr-912 August 31, 2023 16:55 Destroyed

github-actions bot temporarily deployed to preview-pr-912 August 31, 2023 18:31 Destroyed

chenyixuan625 reviewed Sep 5, 2023

View reviewed changes

vinaybhargavar reviewed Sep 6, 2023

View reviewed changes

packages/terra-clinical-item-view/src/ItemView.jsx Show resolved Hide resolved

github-actions bot temporarily deployed to preview-pr-912 September 6, 2023 21:05 Destroyed

github-actions bot temporarily deployed to preview-pr-912 September 7, 2023 19:05 Destroyed

RayGunY requested a review from eawww September 7, 2023 19:08

chenyixuan625 approved these changes Sep 7, 2023

View reviewed changes

vinaybhargavar approved these changes Sep 7, 2023

View reviewed changes

eawww approved these changes Sep 8, 2023

View reviewed changes

sycombs reviewed Sep 11, 2023

View reviewed changes

packages/terra-clinical-item-view/src/ItemView.jsx Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to preview-pr-912 September 11, 2023 21:44 Destroyed

sycombs approved these changes Sep 19, 2023

View reviewed changes

RayGunY force-pushed the UXPLATFORM-9563 branch from ec3f4ef to 62a81c5 Compare September 21, 2023 14:58

github-actions bot temporarily deployed to preview-pr-912 September 21, 2023 15:18 Destroyed

sdadn reviewed Sep 21, 2023

View reviewed changes

ry061521 added 3 commits September 22, 2023 10:58

[clinical-item-view] Add row vs column view prop

c3d71b6

Add 'render' to test titles

cf417ee

Change prop to default to false

456b749

RayGunY force-pushed the UXPLATFORM-9563 branch from 4ebe431 to 456b749 Compare September 22, 2023 15:59

github-actions bot temporarily deployed to preview-pr-912 September 22, 2023 16:09 Destroyed

small test fix

d6edc64

github-actions bot temporarily deployed to preview-pr-912 September 22, 2023 16:19 Destroyed

sdadn mentioned this pull request Sep 22, 2023

[terra-clinical-item-view] Implement item view as unordered list #913

Merged

8 tasks

sdadn reviewed Sep 22, 2023

View reviewed changes

Comment fixes

abe1fee

github-actions bot temporarily deployed to preview-pr-912 September 25, 2023 13:46 Destroyed

sdadn approved these changes Sep 25, 2023

View reviewed changes

sdadn merged commit 8db93c2 into main Sep 25, 2023

sdadn deleted the UXPLATFORM-9563 branch September 25, 2023 17:11

sdadn added the ⭐ UX Reviewed label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[terra-clinical-item-view] Add property to separate by row or column when in two column layout #912

[terra-clinical-item-view] Add property to separate by row or column when in two column layout #912

RayGunY commented Aug 30, 2023 •

edited

Loading

RayGunY Aug 31, 2023

RayGunY Aug 31, 2023

RayGunY commented Sep 8, 2023

sdadn commented Sep 11, 2023

RayGunY commented Sep 11, 2023 •

edited

Loading

RayGunY commented Sep 14, 2023

sdadn Sep 21, 2023

RayGunY Sep 22, 2023

sdadn Sep 22, 2023 •

edited

Loading

sdadn Sep 21, 2023

RayGunY Sep 22, 2023

sdadn commented Sep 21, 2023

RayGunY commented Sep 22, 2023

mjpalazzo commented Sep 22, 2023

RayGunY commented Sep 22, 2023

RayGunY commented Sep 22, 2023

sdadn Sep 22, 2023

RayGunY Sep 25, 2023

sdadn Sep 22, 2023

RayGunY Sep 25, 2023

sdadn Sep 22, 2023

RayGunY Sep 25, 2023

sdadn Sep 22, 2023 •

edited

Loading

sdadn Sep 22, 2023

RayGunY Sep 25, 2023

sdadn commented Sep 22, 2023

RayGunY commented Sep 25, 2023

sdadn commented Sep 25, 2023

		while (displaysSlice.length) {
		displayGroups.push(displaysSlice.splice(0, 2));

	* Added `trueColumn` prop so consumers can choose whether to separate displays in the two column layout by row or by column.
	* Added `trueColumn` prop to toggle between displaying in a two column layout by row or by column.

	* However, this prop defaults to false and therefore the screenreader will read the displays in both columns together, left to right, one row at a time.
	* However, this prop defaults to `false` and therefore the screenreader will read the displays in both columns together, left to right, one row at a time.

	* For accessibility purposes it is recommended to set trueColumn to true.
	* For accessibility purposes it is recommended to set trueColumn to `true`.

		import IconBriefcase from 'terra-icon/lib/icon/IconBriefcase';
		import IconPerson from 'terra-icon/lib/icon/IconPerson';

[terra-clinical-item-view] Add property to separate by row or column when in two column layout #912

[terra-clinical-item-view] Add property to separate by row or column when in two column layout #912

Conversation

RayGunY commented Aug 30, 2023 • edited Loading

Summary

Testing

Reviews

Additional Details

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RayGunY commented Sep 8, 2023

sdadn commented Sep 11, 2023

RayGunY commented Sep 11, 2023 • edited Loading

RayGunY commented Sep 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn commented Sep 21, 2023

RayGunY commented Sep 22, 2023

mjpalazzo commented Sep 22, 2023

RayGunY commented Sep 22, 2023

RayGunY commented Sep 22, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdadn commented Sep 22, 2023

RayGunY commented Sep 25, 2023

sdadn commented Sep 25, 2023

RayGunY commented Aug 30, 2023 •

edited

Loading

RayGunY commented Sep 11, 2023 •

edited

Loading

sdadn Sep 22, 2023 •

edited

Loading

sdadn Sep 22, 2023 •

edited

Loading