feat(admin-ui): Radio & RadioGroup component #4400
Conversation
# Conflicts: # packages/admin-ui/package.json # yarn.lock
| }); | ||
| } | ||
|
|
||
| get id() { |
There was a problem hiding this comment.
Sometimes I wonder do we really need all these getters when they're just returning prop value. 🤔
I guess they're needed because we have private underscored versions of props.
There was a problem hiding this comment.
Although it might seem excessive, having a RadioItem model is very beneficial, and the id getter serves as a good example.
While an id is not required when defining a radio item, it's crucial for consistency in various situations. Ensuring that each RadioItem has a valid id helps us avoid issues when interacting with or displaying items.
If an id isn't provided externally, the model can generate one internally, keeping everything uniform across the application.
| @@ -0,0 +1,8 @@ | |||
| import React from "react"; | |||
There was a problem hiding this comment.
I saw in other file this import is not there, but here we use it.
Just mentioning this for consistency... but yeah.. nitpick.
| id={id} | ||
| className={cn( | ||
| [ | ||
| "group peer aspect-square h-4 w-4 rounded-xl mt-xxs", |
| <RadioGroupPrimitive.Indicator className="flex items-center justify-center"> | ||
| <span | ||
| className={cn([ | ||
| "h-2 w-2 rounded-xl", |
| const DecoratableRadio = React.forwardRef< | ||
| React.ElementRef<typeof RadioGroupPrimitive.Item>, | ||
| RadioProps | ||
| >(({ className, label, id, ...props }, ref) => { |
There was a problem hiding this comment.
Will the users need to pass id all the time in order to have label also clickable / act as if a user clicked on the radio button? 🤔
Can we generate this maybe in cases the user did not pass the ID?
There was a problem hiding this comment.
This component is quite simple; it just receives props and renders the radio button. I modified the props interface to make the id property mandatory.
The id is passed from the RadioItem model, and we know that it will always be a string, even if the user does not provide it.
| /** | ||
| * RadioItem | ||
| */ | ||
| type RadioProps = React.ComponentPropsWithoutRef<typeof RadioGroupPrimitive.Item> & { |
There was a problem hiding this comment.
I noticed you're using type in some of these.
Which is fine, but I believe we're generally leaning more towards using interface when possible. So in this case, we could do:
interface RadioProps extends React.ComponentPropsWithoutRef<typeof RadioGroupPrimitive.Item> {
label: string | React.ReactNode;
}Just mentioning it, it's a nitpick. Bu feel free to leave it if you want.
There was a problem hiding this comment.
Thank you, I have switched to interface in this component. I will implement the same change in other components as well.
| makeAutoObservable(this); | ||
| } | ||
|
|
||
| public init = (params: RadioGroupPresenterParams<TValue>) => { |
There was a problem hiding this comment.
Saw this before and wanted to share an alternative to having to instantiate a class and then call init on it. It would include having a static create class, which takes params and instantiates / inits. So basically:
// Instead of this...
const rgp = new RadioGroupPresenter();
rgp.init(params);
// ...this (the `static` create method creates a new instance of the class):
const rgp = RadioGroupPresenter.create(params);Maybe I missed something though, and this is probably a bit subjective. Still wanted to share, can't hurt. :)
There was a problem hiding this comment.
I'll spend some time experimenting with it; thank you for the suggestion. 😃
|
|
||
| get vm() { | ||
| return { | ||
| items: this.items.map(item => RadioItemMapper.toFormatted(item)) |
There was a problem hiding this comment.
Question I had here before... why is RadioItemMapper having the toFormatted fn. If it's a "mapper", shouldn't it have toMapped method instead?
Don't know what's the correct term/verb here: are we talking about a mapper or a formatter here? 🤔
There was a problem hiding this comment.
It makes sense: the original idea was to have a RadioItemMapper class with multiple methods (toFormatted, toDto...).
I changed it to RadioItemFormatter with a single format method.
| import { RadioItemDto } from "~/Radio/RadioItemDto"; | ||
|
|
||
| interface RadioGroupPresenterParams<TValue = string> { | ||
| items: RadioItemDto[]; |
There was a problem hiding this comment.
Still on the fence whether this is really RadioItemDto. :D
Maybe RadioItemParams?
IDK, I guess LR videos effed my brain and hard-wired the DTO concept to gateway layer 😵 .
There was a problem hiding this comment.
DTOs encapsulate and transfer data between application layers or components, so they are not wired to the gateway layer only.
Anyway, I changed RadioItemDto into RadioItemParams as you suggest. It makes sense 👍
Changes
This pull request adds the
RadioandRadioGroupcomponents to the@webiny/admin-uipackage. Subsequent pull requests will develop the form variant and update the components within the@webiny/ui.How Has This Been Tested?
Manually - Jest