ERM-3894 On "add related agreement" focus should move to "Link agreement" button #1503
Conversation
…ent" button * add focus
|
License CLA Stuck? (Developer should make sure that it is really stuck before clicking) |
* split focus logic
|
EthanFreestone
left a comment
EthanFreestone
left a comment
There was a problem hiding this comment.
@CalamityC Thanks for the proposal here, I think this reinforces what I was hoping not to have to do on the call, but I think we need a more generic way of handling the
- form array new clicked
- focus on element
pattern
| 'buttonRef': triggerButton, | ||
| 'buttonStyle': value ? 'default' : 'primary', | ||
| 'id': `${id}-find-agreement-btn`, | ||
| 'id': triggerButtonId || `${id}-find-agreement-btn`, |
There was a problem hiding this comment.
If we already have a ref, I'm not sure why we need to append a custom id and then focus by that id
|
|
||
| const { selfLinkedWarning, setSelfLinkedWarning } = useLinkedWarning(change, input, parentAgreementId, triggerButton); | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
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
const { focusRef, addCallback } useAddElementFocus
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.
| @@ -0,0 +1,9 @@ | |||
| const focusByIdWhenReady = (id) => { | |||
| requestAnimationFrame(() => { | |||
There was a problem hiding this comment.
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
| @@ -0,0 +1,28 @@ | |||
| import refdata from './refdata'; | |||
|
|
|||
| export const relatedAgreements = [ | |||
There was a problem hiding this comment.
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
add focus handling to RelatedAgreementField