Use component siblings custom hook#611
Conversation
Pull Request Test Coverage Report for Build 5332205595
💛 - Coveralls |
There was a problem hiding this comment.
First and foremost, thanks a ton for taking care of this ❤️
We apologize for the delay in reviewing it; we've been busy with the changes for the June release.
That said, your changes look in the right direction. I left a few comments, but feel free to just add the testing part and ping us again for merging it to master branch as soon as all checks are green.
Do not hesitate to contact us if you have questions about unit testing or filling out the changes file.
| }, [isOpen]); | ||
| if (isOpen) { | ||
| closeButtonRef.current.focus(); | ||
| makeSiblingsInert(); |
There was a problem hiding this comment.
Good catch 👍 As it is now, it will not become siblings as inert if the component is mounted as isOpen in the future (for whatever reason, I have no use case in mind). But your change fixes it.
web/src/hooks/useNodeSiblings.js
Outdated
| if (!node) return [ | ||
| (a, b) => { return [a, b] }, | ||
| (a) => { return [a] } |
There was a problem hiding this comment.
It's ok, but maybe I'd return noop functions instead. Or even skip that check and make siblings an empty array when node is not defined yet.
No strong opinion at this moment, honestly.
| /** | ||
| * Returns two functions, one which adds given attributes to siblings | ||
| * of the node passed as the hook parameter, and another one which removes attributes. | ||
| * | ||
| * @param {HTMLElement} node | ||
| * @returns {Array} | ||
| */ |
There was a problem hiding this comment.
Just a suggestion,
| /** | |
| * Returns two functions, one which adds given attributes to siblings | |
| * of the node passed as the hook parameter, and another one which removes attributes. | |
| * | |
| * @param {HTMLElement} node | |
| * @returns {Array} | |
| */ | |
| /** | |
| * Function for adding an attribute to a sibling | |
| * | |
| * @typedef {function} addAttributeFn | |
| * @param {string} attribute - attribute name | |
| * @param {*} value - value to set | |
| */ | |
| /** | |
| * Function for removing an attribute from a sibling | |
| * | |
| * @typedef {function} removeAttributeFn | |
| * @param {string} attribute - attribute name | |
| */ | |
| /** | |
| * A hook for working with siblings of the node passed as parameter | |
| * | |
| * It returns an array with exactly two functions: | |
| * - First for adding given attribute to siblings | |
| * - Second for removing given attributes from siblings | |
| * | |
| * @param {HTMLElement} node | |
| * @returns {[addAttributeFn, removeAttributeFn]} | |
| */ |
|
Hey @dgdavid Thanks for the review and for the suggestions. I addressed them in the code and made commits accordingly. I've also added unit tests now for the custom hook. Do tell me if anything else is needed for the PR to get merged. |
There was a problem hiding this comment.
Thanks @balsa-asanovic! The only missing piece is adding an entry to the changes file. I would suggest something like this (but feel free to use a different text):
-------------------------------------------------------------------
Wed Jun 21 05:27:53 UTC 2023 - Balsa Asanovic <balsaasanovic95@gmail.com>
- Move the logic to add/remove sibling elements attributes
to a custom hook (gh#openSUSE/agama#565).
|
Hey @imobachgs Thanks for pointing me to the right file. I've added short description of changes, I hope it's OK. |
|
Thank you! It looks OK. I will merge the PRs as soon as the CI runs. |
|
The code is finally merged. @balsa-asanovic Thanks a lot for your contribution! |
|
Great! Thank you guys a lot for the help and guidance. @dgdavid @imobachgs I'll look now for another issue to work on 😃 |
4 participants
Problem
In the Sidebar component an internal function to access its siblings was introduced.
This pull request address issue #565 by creating a custom hook for this purpose and using it instead in Sidebar component.
fixes #565
Solution
The logic of getting HTML node's siblings is moved to a custom hook, where also the functions are introduced for adding and removing attributes to sibling elements. The hook returns these functions as an array. The hook takes an HTML element as input parameter in form of useRef.current.
Testing
At the moment I haven't added any unit tests for the hook, before I get some feedback on the code changes I am not sure if the approach I made is good at all.
I tested the functionality in the app, this works, I can confirm that the inert and aria attributes are being added and removed appropriately.
Screenshots