CNV-18014: Add Size drop down to Add volume modal #1054
Conversation
|
@hstastna: This pull request references CNV-18014 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
hstastna
changed the title
|
@hstastna: This pull request references CNV-18014 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@hstastna: This pull request references CNV-18014 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
hstastna
force-pushed
the
Add_Size_drop_down_instancetype
branch
9 times, most recently
from
8cfdc7d
to
a26edb9
Compare
|
@hstastna: This pull request references CNV-18014 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@hstastna: This pull request references CNV-18014 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@hstastna: This pull request references CNV-18014 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/retest |
hstastna
changed the title
|
@metalice @pcbailey @upalatucci @vojtechszocs please review |
src/views/bootablevolumes/actions/components/EditBootableVolumesModal.tsx
Outdated
Show resolved
Hide resolved
src/views/bootablevolumes/actions/components/EditBootableVolumesModal.tsx
Show resolved
Hide resolved
| const [preference, setPreference] = useState<string>(initialParams.preference); | ||
| const [instanceType, setInstanceType] = useState<string>(initialParams.instanceType); | ||
| const [size, setSize] = useState<string>(initialParams.size); | ||
| const [sizeOptions, setSizeOptions] = useState<string[]>( |
There was a problem hiding this comment.
nitpick: Just for organization purposes, I would move this below the description state to keep all of the state coming from the initialParams object together. You could also use useMemo to have it automatically recalculate whenever instanceType changes instead of setting it manually.
There was a problem hiding this comment.
Okay, I'll move it below the description state. But I am not sure how to combine useState with useMemo for sizeOptions (and if it ever is a good idea)... Or how did you mean that?
There was a problem hiding this comment.
useMemo caches the result of a calculation and returns it on each re-render unless one of its dependencies changes. It's made specifically to watch props and state for changes.
In this case, it would look something like:
const sizeOptions = useMemo(() => instanceTypesToSizeMap[instanceType], [instanceType, instanceTypesToSizeMap]);
You could then remove the call to setSizeOptions as it would no longer be needed.
There was a problem hiding this comment.
I understand what you mean, seems good to me, except that I am not sure if the Size drop down will be re-rendered accordingly, when value of instanceType changes...
There was a problem hiding this comment.
When instanceType changes, the component will re-render. When it re-renders, useMemo will know that it has changed and recompute sizeOptions, which will then be passed to the dropdown causing it to re-render since its props will have changed.
There was a problem hiding this comment.
When instanceType changes, the component will re-render...
Yep, I understand this. I just wouldn't be so sure with anything. I'd better say "it is expected to work like that". Sometimes it doesn't work as we think. React can be tricky sometimes...
In some of my previous code versions of this, I used to update size totally the same way, according to changes of metadata (used to store everything in one object). And even when the value changed, the drop down wasn't re-rendered. Everything looked fine, useMemo was at its right place, too, it didn't work as expected anyway. I had to find some other way of implementation, and fortunately, it seems to be okay now and it DOES re-render! :)
src/views/bootablevolumes/actions/components/EditBootableVolumesModal.tsx
Outdated
Show resolved
Hide resolved
src/views/bootablevolumes/actions/components/EditBootableVolumesModal.tsx
Outdated
Show resolved
Hide resolved
...catalog/CreateFromInstanceTypes/components/AddBootableVolumeModal/AddBootableVolumeModal.tsx
Outdated
Show resolved
Hide resolved
...catalog/CreateFromInstanceTypes/components/AddBootableVolumeModal/AddBootableVolumeModal.tsx
Outdated
Show resolved
Hide resolved
...catalog/CreateFromInstanceTypes/components/AddBootableVolumeModal/AddBootableVolumeModal.tsx
Outdated
Show resolved
Hide resolved
...catalog/CreateFromInstanceTypes/components/AddBootableVolumeModal/AddBootableVolumeModal.tsx
Outdated
Show resolved
Hide resolved
...catalog/CreateFromInstanceTypes/components/AddBootableVolumeModal/AddBootableVolumeModal.tsx
Outdated
Show resolved
Hide resolved
hstastna
force-pushed
the
Add_Size_drop_down_instancetype
branch
3 times, most recently
from
0ff0e83
to
5cda6a3
Compare
...catalog/CreateFromInstanceTypes/components/AddBootableVolumeModal/AddBootableVolumeModal.tsx
Show resolved
Hide resolved
| @@ -2,3 +2,7 @@ export type BootableVolumeMetadata = { | |||
| labels: { [key: string]: string }; | |||
| annotations: { [key: string]: string }; | |||
| }; | |||
|
|
|||
| export type InstanceTypesAndSizesMapType = { | |||
There was a problem hiding this comment.
This name is better, but it's still not as clear as it could be. I did a search for map naming conventions and saw several recommendations for <value type>By<key type>, which I think is very clear since it explains what you're getting and what you're passing to get it. It's also more compact.
So, the type here would be SizesByInstanceType and the map/object name would be sizesByInstanceType, which I think works nicely. What do you think? I'm fine with either that or InstanceTypesToSizes/InstanceTypesToSizesMap.
There was a problem hiding this comment.
I would not use "To" substring in the naming because IMO it is misleading, from the other point of view: we don't convert/change anything to size, size is part of the string/whole information we are dealing with. We just divide the whole label's string in here, and then deal with that some way we need later, but still with the whole information, not with just part of it.
Another note from me is that I don't think it's always good to follow some conventions/general recommendations. IMO readability of the code for a programmer has a higher priority to me (can save a lot of time later etc), and in this case the suggested naming is less clear or self explaining than the original one, when looking at other names and objects in the project.
Anyway, let me reconsider the naming, then I'll update the PR accordingly.
There was a problem hiding this comment.
From what I can tell here, you're taking InstanceType codes such as c4.xlarge and breaking them down into the InstanceType name shorthand (c4) and the size (xlarge). You're then creating a mapping, like below. Correct me if I'm wrong. Perhaps I'm simply misunderstanding what the code is doing here.
const map = {
c4: [xlarge,...],
...
}
If the above example is correct, "To" is a much more accurate term than "and" for this use case. A map connects the values to the keys by mapping the key TO the value. Using "and" implies a group of InstanceTypes and sizes together in one group/array, which is incorrect. Once you access the map using an InstanceType shorthand name, you only get the sizes for that InstanceType category, not a list of sizes AND InstanceType names.
I completely agree that readability is paramount. Conventions typically exist to improve readability not hinder it. I don't see how "XWithY" is clearer than "XtoYMap"/"XToY" or "YByX". I have no idea what "with" is supposed to mean in this instance.
In any case, "Type" shouldn't be used in the name.
There was a problem hiding this comment.
In any case, "Type" shouldn't be used in the name.
I mostly agree, but not 100%. IMO, it depends on a situation...
Anyway, we don't have much time, so can you please check the actual version of the code? I think you are still looking at some old version of the code. It was updated :)
There was a problem hiding this comment.
Weird. I refreshed last night before commenting.
| // eslint-disable-next-line prefer-const | ||
| let instanceTypesAndSizes: InstanceTypesAndSizesObject = {}; | ||
|
|
||
| instanceTypesNames.forEach((instanceType) => { |
There was a problem hiding this comment.
I'm a huge advocate for readability and I can't see an impact to readability here unless the programmer doesn't understand what reduce does, which would be odd for a JavaScript programmer.
In this case, the logic in the body wouldn't really change at all, but it would prevent the requirement for creating an object externally and using side effects from the loop to populate it.
const sizesByInstanceType: SizesByInstanceType = instanceTypeNames.reduce((instanceTypesToSizesMap, instanceType) => {
const [instanceTypeName, size] = instanceType.split('.');
instanceTypesToSizesMap[instanceTypeName] !== undefined
? instanceTypesToSizesMap[instanceTypeName].push(size)
: (instanceTypesToSizesMap[instanceTypeName] = [size]);
});
return instanceTypesToSizesMap;
}, {});
5cda6a3
to
394239c
Compare
Add missing Size dropdown to 'Add volume to boot from' modal when creating a VM, also to 'Edit volume parameters' modal in the Bootable volumes list, to separate workload type and size, and so to set the appropriate Instancetype label to the selected DataSource. Fixes https://issues.redhat.com/browse/CNV-18014, https://issues.redhat.com/browse/CNV-15501
hstastna
force-pushed
the
Add_Size_drop_down_instancetype
branch
from
394239c
to
4efcbd3
Compare
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hstastna, pcbailey The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
| instanceTypesNames.reduce((instanceTypesAndSizes, instanceType) => { | ||
| const [instanceTypePart, sizePart] = instanceType.split('.'); | ||
|
|
||
| instanceTypesAndSizes[instanceTypePart] !== undefined |
There was a problem hiding this comment.
This code could be simplified like so:
instanceTypesAndSizes[instanceTypePart] ??= [];
instanceTypesAndSizes[instanceTypePart].push(sizePart);| setSize(instanceTypesToSizesMap[instanceTypeName][0]); | ||
| }; | ||
|
|
||
| const onChangeVolumeParams = () => { |
There was a problem hiding this comment.
Just a note, any functions passed to React components as props (e.g. callbacks) should be referentially stable across re-renders. Otherwise, you're creating a new function instance on every render here, passing it to other React components and causing re-render of these components.
This can be done via React.useCallback hook https://reactjs.org/docs/hooks-reference.html#usecallback
On the other hand, the extra re-renders aren't normally a big issue, so developers may opt into not doing this.
There was a problem hiding this comment.
React.useCallback was added in some of the followup PRs.
📝 Description
Add missing Size dropdown to Add volume to boot from modal when creating a VM, also to Edit volume parameters (the name is going to be changed to this one in another PR) modal in the Bootable volumes list, to separate workload type and size, and so to set the appropriate InstanceType label to the selected
DataSource.This PR is related to features:
https://issues.redhat.com/browse/CNV-18014
https://issues.redhat.com/browse/CNV-15501
🎥 Demo
Before:
Edit modal in the Bootable volumes list:
No Size drop down in Add volume to boot from modal:
After:
Edit modal in the Bootable volumes list:
Size drop down in Add volume to boot from modal:
After the user has chosen Default InstanceType, the first option of possible appropriate sizes is chosen, selected in the Size drop down, because both values have to be set, if ever (then the user can change the size, of course):