Skip to content

Commit

Permalink
ci: Fix popup tests and ci bugs [skip release] (#1846)
Browse files Browse the repository at this point in the history
Fixed multiple ci failures:
- Popup tests in React 18 due to different `useLayoutEffect` timing
- forward-merge Action not erroring on Chromatic changes
- pull-request Action not failing properly
- `prerelease/minor` canary builds failing to publish

[category:Infrastructure]
  • Loading branch information
NicholasBoll authored Oct 13, 2022
1 parent 1aea636 commit 0677bf8
Show file tree
Hide file tree
Showing 4 changed files with 29 additions and 34 deletions.
1 change: 0 additions & 1 deletion .github/workflows/forward-merge.yml
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ jobs:
appCode: dlpro96xybh
storybookBuildDir: docs
exitOnceUploaded: false
exitZeroOnChanges: true

- name: Start Server
run: npx http-server docs -p 9001 & npx wait-on http://localhost:9001
Expand Down
8 changes: 6 additions & 2 deletions .github/workflows/pull-request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,11 @@ jobs:

verify:
runs-on: ubuntu-latest
if: always()
needs: ['check', 'integration-test', 'visual-test']
steps:
- name: Done
run: echo "Done"
- name: Failure Test
if: contains(needs.*.result, 'failure') || contains(needs.*.result, 'cancelled') # Test the result of all dependencies
run: exit 1
- name: Success
run: exit 0
52 changes: 22 additions & 30 deletions modules/react/popup/lib/hooks/useReturnFocus.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import React from 'react';
import {changeFocus, createElemPropsHook, isFocusable} from '@workday/canvas-kit-react/common';

import {usePopupModel} from './usePopupModel';
import {PopupStack} from '@workday/canvas-kit-popup-stack';

function getScrollParent(element: HTMLElement): HTMLElement {
if (element === document.body || !element.parentElement) {
Expand Down Expand Up @@ -53,19 +54,18 @@ export const useReturnFocus = createElemPropsHook(usePopupModel)(model => {
}
}, []);

const onKeyUp = React.useCallback((event: KeyboardEvent) => {
requiresFocusChangeRef.current = false;
}, []);

// track mousedown events to determine if the mouse target is a focusable element. If it is, we
// should not return focus
const onMouseDown = React.useCallback((event: MouseEvent) => {
elementRef.current = event.target as Element;
}, []);

const onMouseUp = React.useCallback(() => {
elementRef.current = null;
}, []);
const onMouseDown = React.useCallback(
(event: MouseEvent) => {
if (model.state.stackRef.current && event.target instanceof HTMLElement) {
if (!PopupStack.contains(model.state.stackRef.current, event.target)) {
elementRef.current = event.target;
}
}
},
[model.state.stackRef]
);

// We use `useLayoutEffect` because the callback will be called _before_ the browser changes
// focus. This allows the browser to change focus as normal. For example, if the popup closes
Expand All @@ -87,17 +87,14 @@ export const useReturnFocus = createElemPropsHook(usePopupModel)(model => {
return;
}
// capture the element here. The refs will be null by the time the cleanup function is called
const element = (model.state.returnFocusRef || model.state.targetRef).current as HTMLElement | null;
const element = (model.state.returnFocusRef || model.state.targetRef)
.current as HTMLElement | null;
document.addEventListener('mousedown', onMouseDown, true);
document.addEventListener('mouseup', onMouseUp, true);
document.addEventListener('keydown', onKeyDown, true);
document.addEventListener('keyup', onKeyUp, true);

return () => {
document.removeEventListener('mousedown', onMouseDown, true);
document.removeEventListener('mouseup', onMouseUp, true);
document.removeEventListener('keydown', onKeyDown, true);
document.removeEventListener('keyup', onKeyUp, true);
if (!element) {
return;
}
Expand All @@ -108,12 +105,14 @@ export const useReturnFocus = createElemPropsHook(usePopupModel)(model => {
// Don't change focus if user focused on a different element like a `input` or the target
// element isn't on at least halfway rendered on the screen.
if (
(elementRef.current && getFocusableElement(elementRef.current)) ||
elementRect.top + elementRect.height / 2 < scrollParentRect.top ||
elementRect.bottom - elementRect.height / 2 > scrollParentRect.bottom ||
elementRect.left + elementRect.width / 2 < scrollParentRect.left ||
elementRect.right - elementRect.width / 2 > scrollParentRect.right
(elementRef.current && getFocusableElement(elementRef.current)) || // did the user click on a focusable element?
elementRect.top + elementRect.height / 2 < scrollParentRect.top || // is the top half of the target element visible?
elementRect.bottom - elementRect.height / 2 > scrollParentRect.bottom || // is the bottom half of the target element visible?
elementRect.left + elementRect.width / 2 < scrollParentRect.left || // is the left half of the target element visible?
elementRect.right - elementRect.width / 2 > scrollParentRect.right // is the right half of the target element visible?
) {
// reset the focus element and bail early
elementRef.current = null;
return;
}

Expand All @@ -129,17 +128,10 @@ export const useReturnFocus = createElemPropsHook(usePopupModel)(model => {
changeFocus(element);
});
}

elementRef.current = null;
};
}, [
model.state.returnFocusRef,
model.state.targetRef,
visible,
onMouseDown,
onMouseUp,
onKeyDown,
onKeyUp,
]);
}, [model.state.returnFocusRef, model.state.targetRef, visible, onMouseDown, onKeyDown]);

return {};
});
2 changes: 1 addition & 1 deletion utils/publish-canary.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ exec('git diff --name-only HEAD HEAD^')
bump = 'premajor';
} else {
// we'll use `next` for pre minors
preid = 'next';
preid = `${process.env.GITHUB_RUN_NUMBER || 0}-next`;
bump = 'preminor';
}
}
Expand Down

0 comments on commit 0677bf8

Please sign in to comment.