[KED-1450] Move visibleNav/navWidth calculations into redux store #124
Conversation
in mock state.mount
This is so that it would fail properly if the simulated click didn't work
Better to pass an explicitly empty object when values have not been received (and handle the response down the ), than pass false values
|
I think it's best for the front-end engineers to review the changes, so I will leave it to them. |
richardwestenra
changed the title
richardwestenra
changed the title
| className={classnames( | ||
| 'pipeline-sidebar__show-menu pipeline-sidebar__icon-button', | ||
| { | ||
| 'pipeline-sidebar__icon-button--visible': !visible |
There was a problem hiding this comment.
Do we have to pass visible in here? onToggle is already hard-coded to true right. The same applies to HideMenuButton. The reason I'm asking is because 'pipeline-sidebar__icon-button--visible': !visible is a bit hard to parse for my brain, but it's not a biggie.
There was a problem hiding this comment.
Yeah that's a really tricky one - I'm using 'visible' to refer to two different things - the visibility of the sidebar, and the visibility of the Show Menu button, which is only visible when the other is hidden. I think renaming the prop to be more specific will help - I'll do that. Thanks!
src/selectors/layout.js
Outdated
| top, | ||
| outerWidth: width, | ||
| outerHeight: height, | ||
| width: width ? width - sidebarWidth : width, |
There was a problem hiding this comment.
This should be sidebarWidth ? width - sidebarWidth : width (probably)
There was a problem hiding this comment.
ahhhh this is a leftover relic from before I added the check for if (!width || !height) { on line 91. I can now remove the ternary check completely.
"'pipeline-sidebar__icon-button--visible': !visible" was confusing
There is now a guard against width being undefined earlier in the function
Fixes bug introduced in #124
Description
The visibleNav calculation affects the flowchart width and is currently a class method used in multiple places, which is getting difficult to maintain. It's handled in this way because the boolean flag is stored in the Wrapper component state, so it can only be passed down as a prop. Moving this prop to the redux store and using selectors to manage the navWidth calculations will save time and make things more maintainable.
Development notes
I've also added/changed some of the tests to improve coverage and make them more flexible, and done some slight sidebar refactoring.
QA notes
No functionality changes
Checklist
RELEASE.mdfileLegal notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":
I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorised to submit this contribution on behalf of the original creator(s) or their licensees.
I certify that the use of this contribution as authorised by the Apache 2.0 license does not violate the intellectual property rights of anyone else.