-
Notifications
You must be signed in to change notification settings - Fork 10
ERM-3894 On "add related agreement" focus should move to "Link agreement" button #1503
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
Changes from all commits
a658557
cff3284
1acc7e9
6fe5255
1ad3860
8c8c8b7
50d25ad
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,4 @@ | ||
| import React, { useRef } from 'react'; | ||
| import React, { useEffect, useRef } from 'react'; | ||
| import PropTypes from 'prop-types'; | ||
| import { useForm } from 'react-final-form'; | ||
| import { Link } from 'react-router-dom'; | ||
|
|
@@ -46,7 +46,9 @@ const propTypes = { | |
| onAgreementSelected: PropTypes.func.isRequired, | ||
| parentAgreementId: PropTypes.string, | ||
| parentAgreementName: PropTypes.string, | ||
| triggerButtonId: PropTypes.string, | ||
| }; | ||
|
|
||
| const RelatedAgreementField = ({ | ||
| agreement = {}, | ||
| id, | ||
|
|
@@ -55,12 +57,20 @@ const RelatedAgreementField = ({ | |
| onAgreementSelected, | ||
| parentAgreementId, | ||
| parentAgreementName, | ||
| triggerButtonId | ||
| }) => { | ||
| const { change } = useForm(); | ||
| let triggerButton = useRef(null); | ||
|
|
||
| const { selfLinkedWarning, setSelfLinkedWarning } = useLinkedWarning(change, input, parentAgreementId, triggerButton); | ||
|
|
||
| useEffect(() => { | ||
| if (!input.value?.id && triggerButton.current) { | ||
| triggerButton.current.focus(); | ||
| } | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps | ||
| }, []); | ||
|
|
||
| /* istanbul ignore next */ | ||
| const renderLinkAgreementButton = value => ( | ||
| <Pluggable | ||
|
|
@@ -74,7 +84,7 @@ const RelatedAgreementField = ({ | |
| 'aria-haspopup': 'true', | ||
| 'buttonRef': triggerButton, | ||
| 'buttonStyle': value ? 'default' : 'primary', | ||
| 'id': `${id}-find-agreement-btn`, | ||
| 'id': triggerButtonId || `${id}-find-agreement-btn`, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we already have a ref, I'm not sure why we need to append a custom id and then focus by that id |
||
| 'marginBottom0': true, | ||
| 'name': input.name, | ||
| 'onClick': pluggableRenderProps.onClick | ||
|
|
@@ -198,4 +208,3 @@ const RelatedAgreementField = ({ | |
| RelatedAgreementField.propTypes = propTypes; | ||
|
|
||
| export default RelatedAgreementField; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| const focusByIdWhenReady = (id) => { | ||
| requestAnimationFrame(() => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think we should be using animationFrames like this. If we need to wait for a ref to be filled, we can use a callback ref and check whether the current is null or not |
||
| requestAnimationFrame(() => { | ||
| document.getElementById(id)?.focus(); | ||
| }); | ||
| }); | ||
| }; | ||
|
|
||
| export default focusByIdWhenReady; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import focusByIdWhenReady from './focusByIdWhenReady'; | ||
|
|
||
| describe('focusByIdWhenReady', () => { | ||
| let pendingCallbacks; | ||
|
|
||
| beforeEach(() => { | ||
| pendingCallbacks = []; | ||
|
|
||
| global.requestAnimationFrame = jest.fn(cb => { | ||
| pendingCallbacks.push(cb); | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.restoreAllMocks(); | ||
| }); | ||
|
|
||
| test('focuses the element with the given id when ready', () => { | ||
| const focus = jest.fn(); | ||
|
|
||
| jest.spyOn(document, 'getElementById').mockReturnValue({ focus }); | ||
|
|
||
| focusByIdWhenReady('test-id'); | ||
|
|
||
| pendingCallbacks.shift()(); | ||
| pendingCallbacks.shift()(); | ||
|
|
||
| expect(document.getElementById).toHaveBeenCalledWith('test-id'); | ||
| expect(focus).toHaveBeenCalledTimes(1); | ||
| }); | ||
|
|
||
| test('does nothing if the element does not exist', () => { | ||
| jest.spyOn(document, 'getElementById').mockReturnValue(null); | ||
|
|
||
| focusByIdWhenReady('missing-id'); | ||
|
|
||
| pendingCallbacks.shift()(); | ||
| pendingCallbacks.shift()(); | ||
|
|
||
| expect(document.getElementById).toHaveBeenCalledWith('missing-id'); | ||
| }); | ||
| }); |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| import refdata from './refdata'; | ||
|
|
||
| export const relatedAgreements = [ | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I'm reading this right this is just a list of agreements. We should either use the agreements already in the centralised agreements resource, or add these to that list and extract by name/id in the test |
||
| { | ||
| id: '6bcca40d-138c-4c51-b7f5-235fb02552a6', | ||
| note: 'test note 1', | ||
| agreement: { | ||
| id: 'f591806e-fe8c-4b16-a3cb-8cccc180d82b', | ||
| name: 'Related agreement 1', | ||
| agreementStatus: refdata.find(rdc => rdc.desc === 'SubscriptionAgreement.AgreementStatus').values.find(rdv => rdv.value === 'active'), | ||
| startDate: '2021-09-02', | ||
| endDate: null | ||
| }, | ||
| type: refdata.find(rdc => rdc.desc === 'AgreementRelationship.Type').values.find(rdv => rdv.value === 'supersedes').value | ||
| }, | ||
| { | ||
| id: 'ac2fde6e-a6a2-4df5-b244-8d803382d2d5', | ||
| note: 'test note 2', | ||
| agreement: { | ||
| id: 'b958c1be-54f5-4f3d-9131-4255cd21b109', | ||
| name: 'Related agreement 2', | ||
| agreementStatus: refdata.find(rdc => rdc.desc === 'SubscriptionAgreement.AgreementStatus').values.find(rdv => rdv.value === 'in_negotiation'), | ||
| startDate: '2021-09-18', | ||
| endDate: null | ||
| }, | ||
| type: refdata.find(rdc => rdc.desc === 'AgreementRelationship.Type').values.find(rdv => rdv.value === 'provides_post-cancellation_access_for').value | ||
| } | ||
| ]; | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems ok to me on first pass, but I do wonder if we're manually doing this on mount whether we're missing something.
useEffect on mount here I think means that if we decided to open the form up with an empty field instead of none (like periods in agreements) then this would focus.
Whereas what we're actually after is the focusing as a byproduct of being added...
I think alltold this probably should be dealt with more holistically.
Something like
which we can implement within form components, and on creation of a new object handles the focusing of the ref
OR some kind of handling in form arrays specifically, where it internally tracks state, and on addition it can move focus to the new element...
Either way I think controlling from the overview of all elements makes more sense than each element controlling whether it gets focus or not.