OpenNeo/impress-2020
OpenNeo/impress-2020
1
0
Fork 0
Code Pull requests Activity

Stop passing null value in SpeciesColorPicker

Browse source 
This is a minor change to clear a console warning, and make intended behavior clearer! You're not supposed to pass `null` as a select value, because it's ambiguous about whether you're looking for the first option or to make this an "uncontrolled component".

Here, I now provide a fallback value, which is an explicit string for the placeholder option. I made the string very explicit, to aid in debugging if it somehow leaks out from where it's supposed to be! (But I also added gating in the `onChange` event, just to be extra sure.)
This commit is contained in:
Emi Matchu 2021-11-01 16:19:17 -07:00
parent 21096ef837
commit 33740af5ee
1 changed files with 22 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

26
src/app/components/SpeciesColorPicker.js
View file

@ -89,6 +89,13 @@ function SpeciesColorPicker({
newColorId, newColorId,
}); });
// Ignore switching to the placeholder option. It shouldn't generally be
// doable once real options exist, and it doesn't represent a valid or
// meaningful transition in the case where it could happen.
if (newColorId === "SpeciesColorPicker-color-loading-placeholder") {
return;
}
const species = allSpecies.find((s) => s.id === speciesId); const species = allSpecies.find((s) => s.id === speciesId);
const newColor = allColors.find((c) => c.id === newColorId); const newColor = allColors.find((c) => c.id === newColorId);
const validPoses = getValidPoses(valids, speciesId, newColorId); const validPoses = getValidPoses(valids, speciesId, newColorId);
@ -119,6 +126,13 @@ function SpeciesColorPicker({
colorId, colorId,
}); });
// Ignore switching to the placeholder option. It shouldn't generally be
// doable once real options exist, and it doesn't represent a valid or
// meaningful transition in the case where it could happen.
if (newSpeciesId === "SpeciesColorPicker-species-loading-placeholder") {
return;
}
const newSpecies = allSpecies.find((s) => s.id === newSpeciesId); const newSpecies = allSpecies.find((s) => s.id === newSpeciesId);
if (!newSpecies) { if (!newSpecies) {
// Trying to isolate Sentry issue IMPRESS-2020-1H, where an empty species // Trying to isolate Sentry issue IMPRESS-2020-1H, where an empty species
@ -177,7 +191,7 @@ function SpeciesColorPicker({
<Flex direction="row"> <Flex direction="row">
<SpeciesColorSelect <SpeciesColorSelect
aria-label="Pet color" aria-label="Pet color"
value={colorId} value={colorId || "SpeciesColorPicker-color-loading-placeholder"}
// We also wait for the valid pairs before enabling, so users can't // We also wait for the valid pairs before enabling, so users can't
// trigger change events we're not ready for. Also, if the caller // trigger change events we're not ready for. Also, if the caller
// hasn't provided species and color yet, assume it's still loading. // hasn't provided species and color yet, assume it's still loading.
@ -197,7 +211,9 @@ function SpeciesColorPicker({
// placeholder. (Can happen during loading, or if an invalid color ID // placeholder. (Can happen during loading, or if an invalid color ID
// like null is intentionally provided while the real value loads.) // like null is intentionally provided while the real value loads.)
!visibleColors.some((c) => c.id === colorId) && ( !visibleColors.some((c) => c.id === colorId) && (
<option>{colorPlaceholderText}</option> <option value="SpeciesColorPicker-color-loading-placeholder">
{colorPlaceholderText}
</option>
) )
} }
{ {
@ -214,7 +230,7 @@ function SpeciesColorPicker({
<Box width={size === "sm" ? 2 : 4} /> <Box width={size === "sm" ? 2 : 4} />
<SpeciesColorSelect <SpeciesColorSelect
aria-label="Pet species" aria-label="Pet species"
value={speciesId} value={speciesId || "SpeciesColorPicker-species-loading-placeholder"}
// We also wait for the valid pairs before enabling, so users can't // We also wait for the valid pairs before enabling, so users can't
// trigger change events we're not ready for. Also, if the caller // trigger change events we're not ready for. Also, if the caller
// hasn't provided species and color yet, assume it's still loading. // hasn't provided species and color yet, assume it's still loading.
@ -242,7 +258,9 @@ function SpeciesColorPicker({
// ID like null is intentionally provided while the real value // ID like null is intentionally provided while the real value
// loads.) // loads.)
!allSpecies.some((s) => s.id === speciesId) && ( !allSpecies.some((s) => s.id === speciesId) && (
<option>{speciesPlaceholderText}</option> <option value="SpeciesColorPicker-species-loading-placeholder">
{speciesPlaceholderText}
</option>
) )
} }
{ {