feat(subnets): Add form to reserve DHCP lease MAASENG-2975 WIP #5420
Conversation
Demo
Jenkins 
demos.haus 
| name: string; | ||
| }; | ||
|
|
||
| const LimitedIpInput = ({ cidr, name, ...props }: Props) => { |
There was a problem hiding this comment.
Not suggesting you rewrite your implementation, just wanted to point out that there is a library that could help with this in case you weren't aware. Have you looked at it?
https://www.npmjs.com/package/react-input-mask
There was a problem hiding this comment.
I've not heard of that, this could definitely be useful! My only concern is that it was last updated 4 years ago, would be nice if there was something similar that's still in active development
| setInputWrapper(document.querySelector(".limited-ip-input__input-wrapper")); | ||
| setInputElement(document.querySelector(".limited-ip-input__input")); |
There was a problem hiding this comment.
It's generally best to cache selectors and not perform lookup unnecessarily multiple times (document.querySelector).
Also, this will only work as expected if there's a single instance of LimitedIpInput component on the page at a time (lessons learnt from DynamicTable) 😉 . I'd suggest to use ref as an entry point for access and manipulation of individual DOM nodes.
There was a problem hiding this comment.
The only problem with using ref is that the element we're trying to access is the wrapper of the input, rather than the input itself. react-components doesn't let us pass a ref through to the wrapper, only a classname
There was a problem hiding this comment.
Nvm, figured it out - we can put the ref on the top-level div that wraps everything, and then access children from there.
|
|
||
| return ( | ||
| <FormikForm<FormValues> | ||
| aria-label="Reserve static DHCP lease" |
There was a problem hiding this comment.
👍 for not forgetting about accessibility.
| padding-left: $spv--small; | ||
| padding-bottom: calc(0.4rem - 1px); | ||
| padding-top: calc(0.4rem - 1px); | ||
| content: var(--immutable, ""); |
There was a problem hiding this comment.
Very interesting approach, although it makes it somewhat harder to track where the "immutable" comes from as it's hidden in JS logic. This could be also made simpler.
Have you considered using an additional hidden element in JSX encapsulating rendering logic in a single place? Similar idea is used in MainToolbar in maas-react-components: https://github.com/canonical/maas-react-components/blob/main/src/lib/sections/MainToolbar/MainToolbar.tsx#L122-L129
There was a problem hiding this comment.
After our discussion, I've created a jira ticket to investigate a replacement for this logic: https://warthogs.atlassian.net/browse/MAASENG-3116
Honestly, I wasn't sure what it could be used for there, but I'm happy to upstream it regardless. The prop types may be a little looser (e.g. |
OK. For now let's make sure it's a little more generic (partial input/input mask logic is extracted to a separate component which is then used by |
Sounds good to me. I'll address your other comments in this new component. |
petermakowski
added
Review: QA +1
Review: Code +1 (with changes)
and removed
Review: Code needed
Review: QA needed
labels
replace .lastChild.firstChild with querySelector Co-authored-by: Peter Makowski <[email protected]>
Done
ReserveDHCPLeasePrefixedIpInputPrefixedInputQA steps
/subnet/2/address-reservation(any subnet will do)tabFixes
Fixes MAASENG-2975
Screenshots
Notes
PrefixedInputwas initally meant to be a component added to maas-react-components, but due to some strange visual bugs that only occurred in storybook, I've decided to implement it here for now and make the upstreaming future work: https://warthogs.atlassian.net/browse/MAASENG-3113