How necessary is this? Looks like it uses the same variable. I am not understanding what is being set here.

inputProps does not include the id prop, it's only in controlProps. I am not exactly sure how renderControl handles these, but I would assume it's handled differently than here.

However, if that condition fails and the renderControl call isn't returned, the input does not receive an ID. controlProps doesn't actually output the id prop value anywhere on BaseControl.

You can test this by commenting it out and seeing the lack of ID on the input. The intent here was to fix the failure case and be otherwise minimally invasive. I'm not exactly sure what conditions cause renderControl to be falsey or not.

Let me know if you have any other questions.

@talldan I like what I see here but I am still hesitant to approve this as I am not sure about this id thing. Do you know anything about this component? If not, can you request review from someone who might? Overall, I thought we didn't use class components so I am guessing this is one of the first created.

Hi, @iansvo. Thanks for contributing.

I fear that conditionally providing input ID can cause a mismatch when the renderControl override is available. Take the example below; the label here will receive ID but not the input field.

The controlProps and inputProps allow us to set proper attributes without worrying if ID is there or not.

<URLInput
    renderControl={ ( controlProps, inputProps ) => (
        <BaseControl { ...controlProps }>
            <input { ...inputProps } />
        </BaseControl>
    ) }
/>

P.S. Can you rebase this branch on top of the current trunk? 🙇

@Mamaduka I think in my original solution I was just trying to be minimally invasive so I didn't really look into that prop very much. I can see from this PR that this was added as a way to conditional override that rendering (also noted for the future!), which your code here shows.

Now that I understand the intent of this the goal here should just be to always pass the id prop to inputProps and we should have coverage for all cases, including the example case you provided.

The inputId variable is now always present in both controlProps and inputProps. Now, a custom render or a default one will always have everything needed to properly set an accessible label.

This should be fully compatible with previous implementations since the controlProps are unmodified and inputProps only has something new, which is the same value, etc.

I thought about just setting the value in control props and passing it to inputProps by reference, but I think this approach will keep the intent and purpose of the code clear.

Please review the latest commit and let me know if you have any other thoughts/feedback. Thanks again for bringing this up and helping the solution be more holistic!

Thanks for the update, @iansvo.

The changes look good to me 👍