Add options prop to <SelectField>

[joebuono]

Description of changes

The added options prop accepts an array of strings which maps to the value, label and children of each <option> in a SelectField.

If there are children and an options prop is set, then the children will win but there will be a warning in the console.

This requirement will help Amplify Studio's Codegen team create a SelectField easier, plus it has a nice DX if the customer just wants a simple SelectField.

Issue #, if available

Description of how you validated changes

Added a test and confirmed that a new options example in the docs works correctly.

Checklist

• PR description included
• yarn test passes
• Tests are updated
• Relevant documentation is changed or added (and PR referenced)

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

[changeset-bot]

🦋 Changeset detected

Latest commit: 7802de5

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@aws-amplify/ui-react	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[reesscot]

Looks good, just a few comments below

[reesscot]

What do you think of doing an early return for the children, since children takes precedence?

  if (children) {
    if (options?.length) {
          console.warn(
             'Amplify UI: <SelectField> component  defaults to rendering children over `options`. When using the `options` prop, omit children.'
          );
    }
    return children;
  }
  
  return options.map((option, index) => (
        <option label={option} value={option} key={index}>
          {option}
        </option>
  ));

IMO, else statements are usually a code smell that the logic should be simplified (often using an early return).

[joebuono]

Good point. The reason I wrote it this way is because I wanted to handle the case where there are neither children nor options. By default, I think we want it to return children.

[dbanksdesign]

Great job!

[dbanksdesign]

It would be nice to add an example in here too.

[dbanksdesign]

https://caniuse.com/mdn-javascript_operators_optional_chaining

Oh we can use javascript optional chaining??

[reesscot]

Yeah, and I believe TypeScript should handle the conversion to ES5 if not.

[reesscot]

LGTM

[zchenwei]

Should we mark it as a minor bump? IMO, patch is for small bug fixes 🤣

[joebuono]

Yes, good idea

[zchenwei]

Suggested change

	if (children) {
	if (options?.length) {
	if (children) {
	if (Array.isArray(options)) {
	....
	}

nit: do you think this would be better?

[joebuono]

If there's a way to handle all four of these cases without nested if...else blocks, then yes:

1. There are children, but no options
2. There are children and options
3. There are no children and no options
4. There are no children, but there are options

[zchenwei]

Right, my bad. I missed something there and I updated the comment there :)

[zchenwei]

Just think out loud, I think appending index will somehow affect React's diff algorithm negatively. This can work well if the items are never reordered, but reorders will be slow. For example, if I have an option <option label="amplify" value="amplify" key={'amplify-2'}> at index 2, and I make updates later to the option array, where I get the exact same option <option label="amplify" value="amplify" key={'amplify-1'}> move to index 1. However, React will think they are different because their keys are affected by the index and thus re-created it versus keeping the original one

[zchenwei]

Good reading <- https://reactjs.org/docs/reconciliation.html