MEN-9212: release filter improvements#1969
Conversation
mineralsfree
force-pushed
the
MEN-9212
branch
2 times, most recently
from
24387ad to
ceff016
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
mzedel
left a comment
mzedel
left a comment
There was a problem hiding this comment.
👍 I like the proximity of the ControlledSelect to the AutoComplete 😁 - only the duplicated option.key === softwareKindOptions.manifest.key && !isEnterprise would be slightly risky down the line IMO, but apart from that 👌 !
| return ( | ||
| <> | ||
| <h4>Select software to deploy</h4> | ||
| <div className="flexbox margin-bottom-x-small"> |
There was a problem hiding this comment.
| <div className="flexbox margin-bottom-x-small"> | |
| <div className="flexbox align-items-center margin-bottom-x-small"> |
here and with the others - I guess with the small tooltips it's not that much of an issue though
| kind: debouncedKind ?? undefined, | ||
| selectedTags: tags, | ||
| type: debouncedKind === softwareKindOptions.manifest.key ? undefined : (type ?? undefined) | ||
| type: debouncedKind === softwareKindOptions.manifest.key ? undefined : type || undefined |
There was a problem hiding this comment.
| type: debouncedKind === softwareKindOptions.manifest.key ? undefined : type || undefined | |
| type: debouncedKind === softwareKindOptions.manifest.key || !type ? undefined : type |
not 100% sure if that's more readable, but both the prior ?? as well as the || to undefined read weird to me 🤷
| placeholder="Select type" | ||
| hideEmptyOption | ||
| width={270} | ||
| MenuProps={{ slotProps: { paper: { sx: { width: 270, '& .MuiMenuItem-root.Mui-disabled': { opacity: 1 } } } } }} |
There was a problem hiding this comment.
can we have this in a class above & reference the menuitem via the mui class references? both the MenuProps or the ControlledSelect itself would accept a className
| getOptionLabel={(value: SoftwareKind) => softwareKindOptions[value].title} | ||
| getOptionDisabled={(option: SoftwareKind) => option === softwareKindOptions.manifest.key && !isEnterprise} | ||
| renderInput={params => <TextField {...params} label="Select type" placeholder="Select type" />} | ||
| options={Object.values(softwareKindOptions)} |
There was a problem hiding this comment.
maybe map the softwareKindOptions values before to include the disabled state - since both getOptionDisabled and renderOption check for isEnterprise and thus bring in more places this check can diverge
| const locked = option.key === softwareKindOptions.manifest.key && !isEnterprise; | ||
| return ( | ||
| <span className={classes.kindOption}> | ||
| <span className={locked ? 'muted' : undefined}>{option.title}</span> |
There was a problem hiding this comment.
| <span className={locked ? 'muted' : undefined}>{option.title}</span> | |
| <Typography color={option.disabled ? 'text.disabled' : 'inherit'}>{option.title}</Typography> |
maybe?
| renderOption={(option: SoftwareKindOption) => { | ||
| const locked = option.key === softwareKindOptions.manifest.key && !isEnterprise; | ||
| return ( | ||
| <span className={classes.kindOption}> |
There was a problem hiding this comment.
maybe:
kindOptions: { gap: theme.spacing(2) }
and then:
| <span className={classes.kindOption}> | |
| <div className={`flexbox align-items-center ${classes.kindOption}`}> |
? - did not try this though... 😬
Ticket: MEN-9212 Signed-off-by: Mikita Pilinka <mikita.pilinka@northern.tech>
… adjustable Ticket: MEN-9212 Signed-off-by: Mikita Pilinka <mikita.pilinka@northern.tech>
+ disabled option support and hide empty option controlled with props Ticket: MEN-9212 Signed-off-by: Mikita Pilinka <mikita.pilinka@northern.tech>
Ticket: MEN-9212 Signed-off-by: Mikita Pilinka <mikita.pilinka@northern.tech>
Ticket: MEN-9212 Signed-off-by: Mikita Pilinka <mikita.pilinka@northern.tech>
Ticket: MEN-9584 Signed-off-by: Mikita Pilinka <mikita.pilinka@northern.tech>
Ticket: MEN-9584 Signed-off-by: Mikita Pilinka <mikita.pilinka@northern.tech>
Ticket: MEN-9212 Signed-off-by: Mikita Pilinka <mikita.pilinka@northern.tech>
2 participants
Needs