Bug 2175990: Improve Default InstanceType field in Edit modal #1232
Conversation
|
@hstastna: This pull request references Bugzilla bug 2175990, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: 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 Bugzilla bug 2175990, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
@avivtur @metalice @pcbailey @upalatucci @vojtechszocs please review |
hstastna
force-pushed
the
BZ_Improve_Edit_dialog_InstanceType_field
branch
from
0bf1a38
to
fcbd1bc
Compare
| for (const category of Object.entries(categoryDetailsMap)) { | ||
| if (category[1].prefix === instanceTypeLabel?.[0]) { | ||
| return category; |
There was a problem hiding this comment.
nit suggestion:
Object.entries(categoryDetailsMap).map(category => category[1].prefix === instanceTypeLabel?.[0] && category)
There was a problem hiding this comment.
Yep, now I can see I could simply use find, but...
nit suggestion:
Object.entries(categoryDetailsMap).map(category => category[1].prefix === instanceTypeLabel?.[0] && category)
Not sure why but this didn't work for all the cases I've tested. So I better used the less optimized version that works perfectly.
Note that when I was trying to enhance the version with && asi Aviv suggested, to make it work for all the cases we need, it went even worse than my "less optimized version".
But I'll try to take a bit time for this, maybe I'll find something else and better.
There was a problem hiding this comment.
There isn't an issue here of less or more optimized. If u are using find u don't need && category. Find callback just need to meet the condition.
There was a problem hiding this comment.
It will return the first element that meets the condition successfully
There was a problem hiding this comment.
Yep, right, I've realized that just after I sent the comment :D
Just a note I am still working on that, want to test my improved code but I am blocked by some other bug - I cannot create a bootable volume (and I do need, with specific labels, and cannot edit labels of other existing ones), that has occurred unexpectedly just from today on our main branch..
| optionLabelText={t('size')} | ||
| /> | ||
| <Select | ||
| menuAppendTo="parent" |
There was a problem hiding this comment.
please check if this doesn't cause any bugs, such as the dropdown sometimes opening up instead of down, anyway, I'm not sure u need this line
There was a problem hiding this comment.
I've checked that and we DO need it, because it looks better and it prevents the user's need of unnecessary scrolling across the options, see the difference:
-
without
menuAppendTo="parent":
-
with
menuAppendTo="parent":
There was a problem hiding this comment.
Great just make sure that it ain't open up on some resolution on chrome and firefox. I encounter this bug with menu append
There was a problem hiding this comment.
I will. Anyway, IMO the benefit of using menuAppendTo="parent" in our case is bigger than its downside - potential opening the drop down up in some cases, as you've mentioned (and I'd consider that as a feature, not a bug, if there was much more space above than below the component).
There was a problem hiding this comment.
Tested in various browsers - Firefox, Chrome, Chromium, different resolutions and size of the browser window, works as expected.
src/views/bootablevolumes/actions/components/EditBootableVolumesModal.tsx
Outdated
Show resolved
Hide resolved
src/views/bootablevolumes/actions/components/EditBootableVolumesModal.tsx
Outdated
Show resolved
Hide resolved
hstastna
force-pushed
the
BZ_Improve_Edit_dialog_InstanceType_field
branch
2 times, most recently
from
ff60825
to
78fd61a
Compare
|
/hold |
hstastna
force-pushed
the
BZ_Improve_Edit_dialog_InstanceType_field
branch
from
78fd61a
to
dcaecdb
Compare
Improve 'Default InstanceType' field/dropdown in Edit volume metadata modal accessible in Bootable volumes list, according to the design update: add descriptive text to each one, omit "highperformance" and "server" options from the dropdown. Improve also related 'Size' dropdown to display items with more info about number of CPUs and Memory size. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=2175990
hstastna
force-pushed
the
BZ_Improve_Edit_dialog_InstanceType_field
branch
from
dcaecdb
to
db66211
Compare
|
/hold cancel |
|
@avivtur @metalice @pcbailey @upalatucci @vojtechszocs please review |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: hstastna, vojtechszocs 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 |
|
/hold I have a few comments, but haven't finished my review. |
|
@hstastna: All pull requests linked via external trackers have merged: Bugzilla bug 2175990 has been moved to the MODIFIED state. 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. |
|
|
||
| return { | ||
| preference: dataSource?.metadata?.labels?.[DEFAULT_PREFERENCE_LABEL], | ||
| instanceType: instanceTypeLabel?.[0], | ||
| size: instanceTypeLabel?.[1], | ||
| instanceType: initialCategory, |
There was a problem hiding this comment.
This field should be renamed to something like category or instanceTypeCategory as an instanceType refers to a specific combination of category and size. We should have used called categories "series" as that's what the documentation says. I'm not sure why we didn't.
There was a problem hiding this comment.
I was already addressing this problem of naming these kinds of data to Matan. The problem is we are not sure how to call what. So each of us - devs, uses their own namings... And this feature is still evolving... And the UI is misleading with this, too, as "Default InstanceType" field in the Add volume and Edit volume metadata modals refers to only first part of the whole name of the InstanceType resource (before "."). But that was not our decision....
We should come up with some convention for us about how to name specific data related to InstanceTypes in our codebase, so we could understand the code better, at least us. And then I could make an update, of course.
IMO the core of all these problems is the design of the InstanceType concept, where 2 different data is merged in one name for InstanceType resource, while in the UI it is shown separately (which can IMO produce just more misleading feelings for our users, as soon as they realize that those 2 "things" are just one resource). It makes our lives in the UI harder. But I was told it was like that because it is easier for the API like that 🤷🏼♀️
There was a problem hiding this comment.
I definitely think more discussion should have been had around the nomenclature for this feature before it was implemented. I've been going by Fabian's documentation for the feature, here: https://github.com/fabiand/instanceTypes/blob/main/instanceTypes.md. It's the best I've seen so far in terms of explaining the terminology.
| const initialParams = useMemo(() => { | ||
| const instanceTypeLabel = | ||
| dataSource?.metadata?.labels?.[DEFAULT_INSTANCETYPE_LABEL]?.split('.'); | ||
| const initialCategory = Object.entries(categoryDetailsMap).find( |
There was a problem hiding this comment.
I couldn't find where the details part of the pair is used. If it isn't needed, then only the category name should be returned. Like this:
const initialCategory = Object.keys(categoryDetailsMap).find(
(category) => categoryDetailsMap[category].prefix === instanceTypeLabel?.[0],
);
There was a problem hiding this comment.
Looks perfect. Just a little issue in here: not sure why (I don't remember now already) but this wasn't working as expected for all the cases I was testing. There was something problematic about how the categoryDetailsMap is built (and IMHO it is a bit more complex than needed).
There was a problem hiding this comment.
I pulled your PR and tested it myself, but I don't know what all you did to test it. categoryDetailsMap definitely holds a lot of information. In retrospect, I probably should have created one map for the category information and another one for the instanceTypes. I think that would help a bit. I'm open to other suggestions.
There was a problem hiding this comment.
No worries, we will figure that out. And I'll try to test it again to make sure. And will create a followup PR if needed.
| [instanceType, instanceTypesToSizesMap], | ||
| // update options for 'Size' dropdown according to chosen 'instanceType' in 'Default InstanceType' dropdown | ||
| const { instanceTypes }: CategoryDetails = useMemo( | ||
| () => (instanceType ? categoryDetailsMap[instanceType] : {}), |
There was a problem hiding this comment.
This is a good example of where self-documenting code can replace a comment. This function body can be extracted to a function with a descriptive name like getCategoryInstanceTypes or getInstanceTypesForCategory. You could also rename the variable to instanceTypeOptions if you'd like.
There was a problem hiding this comment.
This is the problem of naming the data we are working with, as I've already mentioned in another comment... I'm not gonna change that till we come up with some convention for names of the data we are working with, regarding InstanceTypes, because instanceTypeOptions would IMHO still not be good enough, as InstanceType resource is not what we call "InstanceType" in the UI.
There was a problem hiding this comment.
I understand that you don't like the naming as it is right now. That's still no excuse to use comments of self-documenting code. The naming can be updated if/when the nomenclature is clarified.
There was a problem hiding this comment.
Seems like we have different opinion on if my comment in this place is appropriate or not. I was thinking about these names and stuff for a long time, when working on this PR. And I decided to better put the comment in this place. I would go against myself and other good habits of one developer if I simply ignored the fact of our naming issue and not added that comment at all. Or if I used long ugly unreadable name like DefaultInstanceTypeDropdownOptions, because that would be the most appropriate to describe what is going on there 😇 With such long names everywhere in our code, we wouldn't help us much. I'll better add one readable comment, in this specific case. Do we have to make it ourselves even harder if we don't have to?
Once we get to some clear naming convention for InstanceTypes related data, the comment will become unnecessary and will be removed, together with updating related namings we use in our code.
We can change everything we want. That means we can add or remove any comment we want, too. That's nothing permanent in our code. So please take it as a temporary improvement of what we have just now. Btw, I'd like to see more comments in our code than we have now. Because there are the same naming issues also in other areas, and absence of comments in some places makes (not just) my life just harder, slows down the development... Using comments isn't bad. It's a tool. We should use the tools we have if they help us, if the actual upside of the used tool is higher than its downside. When using too many unnecessary comments - that's not helpful, of course. Unfortunately, this is obviously subjective when some comment is good to use and when not. I think there is no any clear way how to solve that universally, except hiring a different dev who would be more aligned with opinions and experience of other devs in the team 😄
There was a problem hiding this comment.
We should avoid long names if we can use a shorter one that communicates the necessary information, but using a longer name that communicates that information is superior to a comment as it will clear anywhere it's used in the code and not just in that one spot. In general, if a comment is just stating what the code is doing and the code isn't something that's unusually hard to parse (like some type of non-idiomatic code or regex), the solution is to refactor, not comment.
I just looked at it again and realized this block of code could be clearer if you renamed the instanceType state to category as suggested above.
const instanceTypes: InstanceTypeSizeDetails[] = useMemo(
() => (category ? categoryDetailsMap[category].instanceTypes : {}),
[category],
);
I'll better add one readable comment, in this specific case. Do we have to make it ourselves even harder if we don't have to?
I see you use this reasoning a lot. It's better to avoid using a comment, if possible, than use it as a crutch to avoid using practices that aid code readability like good naming, extracting logic into methods that can benefit from good naming, etc.
We can change everything we want. That means we can add or remove any comment we want, too. That's nothing permanent in our code. So please take it as a temporary improvement of what we have just now.
It's true that we can modify the code as we see fit. We should still always opt to make the best changes possible. In this case, there were better choices than the comment as I pointed out above.
I'd like to see more comments in our code than we have now. Because there are the same naming issues also in other areas
If there are areas that you think need comments or that suffer from naming issues, then that means the code likely needs to be refactored. It's more likely that the issue is a code smell than a scenario where a comment is the best solution. I suggest you put together a PR with the comments you feel are necessary and let the team review it. It will likely result in lots of opportunity for refactoring.
| const preferenceLabel = preference && { [DEFAULT_PREFERENCE_LABEL]: preference }; | ||
|
|
||
| const categoryObject = categoryDetailsMap[instanceType]; |
There was a problem hiding this comment.
Using Object at the end of the sentence usually doesn't communicate much useful information. It's better to just use a name that helps to identify what type of data that object holds. In this case, this object is an instance of a type, CategoryDetails, so it makes since to use the type name as the variable name.
There was a problem hiding this comment.
Will improve that in some followup PR.
📝 Description
Fixes:
https://bugzilla.redhat.com/show_bug.cgi?id=2175990
Improve Default InstanceType field/dropdown in Edit volume metadata modal, accessible in Bootable volumes list, according to the design update: add descriptive text to each one, omit "highperformance" and "server" options from the dropdown.
Improve also related 'Size' dropdown to display items with more info about number of CPUs and Memory size, sorted according to the size and not alpabetically, as expected.
See more for Default InstanceType and Size drop downs design here:
https://docs.google.com/document/d/1HTF6_H2WDXwlVy7RnrHH28dzFlhNIa15Hhpeoyg2R0Q/edit
🎥 Screenshots
Before:
Not much info about options for Default InstanceType, also the selected option not marked as selected in the dropdown:
Sizes sorted alphabetically:
After:
