upcoming: [M3-7925] - Linode Create Refactor - Add Firewall - Part 5 #10315
Conversation
bnussman-akamai
requested review from
carrillo-erik,
hkhalil-akamai,
jaalah-akamai and
abailly-akamai
and removed request for
a team
| <Autocomplete | ||
| disabled={isLinodeCreateRestricted} | ||
| errorText={fieldState.error?.message ?? error?.[0].reason} | ||
| label="Assign Firewall" | ||
| loading={isLoading} | ||
| noMarginTop | ||
| onChange={(e, firewall) => field.onChange(firewall?.id ?? null)} | ||
| options={firewalls ?? []} | ||
| placeholder="None" | ||
| value={selectedFirewall} | ||
| /> |
There was a problem hiding this comment.
I'm tempted to build a FirewallSelect. Thoughts?
There was a problem hiding this comment.
Not a bad idea considering what we've done for others
There was a problem hiding this comment.
Would SelectFirewallPanel be applicable here?
There was a problem hiding this comment.
We could use SelectFirewallPanel and just wrap it in a controller, but I've been trying to make the new create flow more "pure".
I decided to write a new component to avoid the extra mapping and one off styling found in SelectFirewallPanel. We also get the ability to use useController without needing a "wrapping" component.
I'm open to re-using SelectFirewallPanel but I think I slightly prefer a dedicated component.
There was a problem hiding this comment.
We'll probably use the FirewallSelect is several places too, so I'm good with creating a new component.
There was a problem hiding this comment.
Code looks good and clean! Couple things to address:
- We're missing the firewall autocomplete value when creating the firewall on the spot (does not populate the select after having been created). This is a regression with the current behavior
- The placeholder for the select says "None", others on the create flow say ex: "Select a Placement Group". Coming to think of it, "None" feels right but we should pick one and be consistent.
| <Autocomplete | ||
| disabled={isLinodeCreateRestricted} | ||
| errorText={fieldState.error?.message ?? error?.[0].reason} | ||
| label="Assign Firewall" | ||
| loading={isLoading} | ||
| noMarginTop | ||
| onChange={(e, firewall) => field.onChange(firewall?.id ?? null)} | ||
| options={firewalls ?? []} | ||
| placeholder="None" | ||
| value={selectedFirewall} | ||
| /> |
There was a problem hiding this comment.
Not a bad idea considering what we've done for others
|
I'm unable to reproduce 1 @abailly-akamai For 2, I just copied the existing placeholder of "None". I don't mind it, but I can add this to the list of things to bring up with UX! |
|
@bnussman-akamai i see what happened with #1 - i was on Alpha and it took a couple seconds to appear. Unrelated to this PR and not blocking, but we could consider adding loading state for those selects eventually |
| <Autocomplete | ||
| disabled={isLinodeCreateRestricted} | ||
| errorText={fieldState.error?.message ?? error?.[0].reason} | ||
| label="Assign Firewall" | ||
| loading={isLoading} | ||
| noMarginTop | ||
| onChange={(e, firewall) => field.onChange(firewall?.id ?? null)} | ||
| options={firewalls ?? []} | ||
| placeholder="None" | ||
| value={selectedFirewall} | ||
| /> |
There was a problem hiding this comment.
We'll probably use the FirewallSelect is several places too, so I'm good with creating a new component.
|
Created M3-7933 for building a FirewallSelect |
Description 📝
Preview 📷
How to test 🧪
Prerequisites
Linode Create v2feature flag using the local dev toolsVerification steps
As an Author I have considered 🤔