Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[HTMLSelect] fix option labels display & use value as label if not specified #2679

Closed
wants to merge 2 commits into from

Conversation

jaamison
Copy link
Contributor

Changes proposed in this pull request:

This PR addresses 2 related but different issues with the core/HTMLSelect component:

1) Options' labels are not being rendered

I first noticed this on the Popover example page:
screen shot 2018-07-11 at 12 22 48 pm

Apparently, React (16.2 at least) does not respect use of the label HTML attribute in lieu of child text nodes for <option> elements (upstream bug?).

This PR reverts to specifying labels for <option> items as inner HTML, rather than specifying them via the element's label attribute, which appears to display correctly:
screen shot 2018-07-11 at 12 23 11 pm

2) If consumer supplies an array of {value, label} plain objects as options={...} , the component does not fallback to using the value of value as the label in cases where label is either not supplied or is falsy.

I understand that it might seem logical to put the burden of ensuring proper argument syntax on the consumer, but I can foresee cases where this fallback logic would avoid unpleasant surprises, even for consumers who are aware of the proper syntax.

For instance, if consumer supplies:

options={ [{value:1, label:"one"}, {value:2}, {value:3, label:"three"}, {value:4, label:undefined}] }

the the current behavior would be to render options 2 & 4 as empty. The logic in this PR would at least render "2" & "4" respectively.

Copy link
Contributor

@giladgray giladgray left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wholly too much code. the existing map is almost enough:

        const optionChildren = options.map(option => {
            const { value, label }: IHTMLOptionProps = typeof option === "object" ? option : { value: option };
            return (
                <option key={value} value={value}>
                    {label || value}
                </option>
            );
        });

return {
label: option.label || option.value.toString(),
value: option.value,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this function is too similar to lines 85-88. DRY please.

@giladgray
Copy link
Contributor

pushed my code above to #2681

@giladgray giladgray closed this Jul 11, 2018
@giladgray
Copy link
Contributor

@jaamison thanks for pushing this up and pointing out these issues! i fixed the component myself in the interest of time -- ready to cut the patch release asap.

@jaamison
Copy link
Contributor Author

awesome, much more concise

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants