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

[EuiPageHeader] rightSideItems Update DOM to Match Display Order #6753

Merged
merged 10 commits into from
May 5, 2023
Merged

[EuiPageHeader] rightSideItems Update DOM to Match Display Order #6753

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
c041438
[EuiPageHeader] Removed the default row-reverse flex-direction for ri…
breehall May 3, 2023
13af141
CHANGELOG
breehall May 4, 2023
6997308
Update upcoming_changelogs/6753.md
breehall May 4, 2023
c34fd38
Update src/components/page/page_header/page_header_content.tsx
breehall May 4, 2023
12aaaea
[REVERT ME] Add responsive prop to the base EuiPageHeader for testing…
breehall May 4, 2023
8e96212
Update snapshots for code changes that occurred in Github UI
breehall May 4, 2023
8de8579
Revert "[REVERT ME] Add responsive prop to the base EuiPageHeader for…
breehall May 4, 2023
e6ef7a7
Update src/components/page/page_header/page_header_content.tsx
breehall May 4, 2023
9bd93f5
Update src/components/page/page_header/page_header_content.tsx
breehall May 4, 2023
0aa50f1
Merge branch 'pageheader/right-side' of https://github.com/breehall/e…
breehall May 4, 2023
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
22 changes: 11 additions & 11 deletions src/components/page/page_header/__snapshots__/page_header.test.tsx.snap
View file
Open in desktop
cee-chen marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -41,20 +41,20 @@ exports[`EuiPageHeader props alignItems bottom is rendered 1`] = `
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 1
Button 2
</button>
</div>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 2
Button 1
</button>
Comment on lines +44 to 58
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This snapshot change is expected. Previously with flex, in the DOM the array would be documented as [1,2], but displayed as [2,1].

Now the DOM order matches the display order.

</div>
</div>
Expand Down Expand Up @@ -87,7 +87,7 @@ exports[`EuiPageHeader props alignItems center is rendered 1`] = `
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
Expand Down Expand Up @@ -133,20 +133,20 @@ exports[`EuiPageHeader props alignItems stretch is rendered 1`] = `
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 1
Button 2
</button>
</div>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 2
Button 1
</button>
</div>
</div>
Expand Down Expand Up @@ -179,7 +179,7 @@ exports[`EuiPageHeader props alignItems top is rendered 1`] = `
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
Expand Down Expand Up @@ -362,21 +362,21 @@ exports[`EuiPageHeader props page content props are passed down is rendered 1`]
>
<div
aria-label="aria-label"
class="euiFlexGroup testClass1 testClass2 emotion-euiFlexGroup-responsive-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup testClass1 testClass2 emotion-euiFlexGroup-responsive-wrap-l-flexStart-stretch-row"
data-test-subj="test subject string"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 1
Button 2
</button>
</div>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 2
Button 1
</button>
</div>
</div>
Expand Down
28 changes: 14 additions & 14 deletions src/components/page/page_header/__snapshots__/page_header_content.test.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,20 +36,20 @@ exports[`EuiPageHeaderContent props alignItems bottom is rendered 1`] = `
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 1
Button 2
</button>
</div>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 2
Button 1
</button>
</div>
</div>
Expand Down Expand Up @@ -78,7 +78,7 @@ exports[`EuiPageHeaderContent props alignItems center is rendered 1`] = `
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
Expand Down Expand Up @@ -120,20 +120,20 @@ exports[`EuiPageHeaderContent props alignItems stretch is rendered 1`] = `
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 1
Button 2
</button>
</div>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 2
Button 1
</button>
</div>
</div>
Expand Down Expand Up @@ -162,7 +162,7 @@ exports[`EuiPageHeaderContent props alignItems top is rendered 1`] = `
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
Expand Down Expand Up @@ -321,20 +321,20 @@ exports[`EuiPageHeaderContent props children is rendered even if content props a
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 1
Button 2
</button>
</div>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 2
Button 1
</button>
</div>
</div>
Expand Down Expand Up @@ -500,20 +500,20 @@ exports[`EuiPageHeaderContent props rightSideItems is rendered 1`] = `
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<div
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-rowReverse"
class="euiFlexGroup emotion-euiFlexGroup-wrap-l-flexStart-stretch-row"
>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 1
Button 2
</button>
</div>
<div
class="euiFlexItem emotion-euiFlexItem-growZero"
>
<button>
Button 2
Button 1
</button>
</div>
</div>
Expand Down
39 changes: 24 additions & 15 deletions src/components/page/page_header/page_header_content.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -344,25 +344,34 @@ export const EuiPageHeaderContent: FunctionComponent<EuiPageHeaderContentProps>

let rightSideFlexItem;
if (rightSideItems && rightSideItems.length) {
const wrapWithFlex = () => {
return rightSideItems.map((item, index) => {
return (
<EuiFlexItem grow={false} key={index}>
{item}
</EuiFlexItem>
);
});
const wrapWithFlex = (reverse: boolean) => {
const reverseRightSideItems = reverse
? rightSideItems.reverse()
Copy link
Contributor

Choose a reason for hiding this comment

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

.reverse() mutates the array in place. Do not ever call it without shallow cloning the array first, especially on arrays coming from consumers (vs arrays we define/control).

Suggested change
const reverseRightSideItems = reverse
? rightSideItems.reverse()
const reverseRightSideItems = reverse
? [...rightSideItems].reverse()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do not ever call it without shallow cloning the array first

Thank you for this and duly noted!

Copy link
Contributor

@cee-chen cee-chen May 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify, I was being a little didactic with the initial "do not ever". If we're the ones defining an array and know we want to simply mutate/reverse the array in place, great, mutate away (although some schools of thought in JS would still say don't do this :).

In this case rightSideItems is not an array we define/own and we do not want ever want to mutate consumer data.

: undefined;

if (reverseRightSideItems) {
return reverseRightSideItems.map((item, index) => {
return (
<EuiFlexItem grow={false} key={index}>
{item}
</EuiFlexItem>
);
});
} else {
return rightSideItems.map((item, index) => {
return (
<EuiFlexItem grow={false} key={index}>
{item}
</EuiFlexItem>
);
});
}
breehall marked this conversation as resolved.
Show resolved Hide resolved
};

rightSideFlexItem = (
<EuiFlexItem grow={false}>
<EuiFlexGroup
wrap
responsive={false}
direction={isResponsiveBreakpoint ? undefined : 'rowReverse'}
{...rightSideGroupProps}
>
{wrapWithFlex()}
<EuiFlexGroup wrap responsive={false} {...rightSideGroupProps}>
{wrapWithFlex(isResponsiveBreakpoint ? false : true)}
breehall marked this conversation as resolved.
Show resolved Hide resolved
</EuiFlexGroup>
</EuiFlexItem>
);
Expand Down
2 changes: 2 additions & 0 deletions upcoming_changelogs/6753.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Improved accessibility in `EuiPageHeader` by ensuring the right side menu items come into focus from left to right.
breehall marked this conversation as resolved.
Show resolved Hide resolved