I've noticed we're using the generic prop name value in a number of these components. Would it make sense to make it something more descriptive/explicit? If I'm working in several of them, it can become a bit confusing.

The reason for doing this, is that I wanted to follow the same format as an InputField (envisioning that every field has a value prop and a onChange handler.
This way, if we go the schema route for example it will be much easier to integrate these field components within a form and we don't have to add a custom alias for each one.

I know this has mostly been something I have been adhering to, but it hasn't really been a team discussion, I could mention this in Slack if you think that is valuable, depending on what you think of this approach?

@louwie17 When you say "going the schema route," are you talking in terms of documentation? I'm not all too familiar with how that would look; it sounds like an alias within the component (while keeping the value prop name) would still influence the schema in this way?

That being the case, sounds good to me. Although this should definitely be something that we make the team aware of, since it would require consistency in order to be more valuable.

When you say "going the schema route," are you talking in terms of documentation?

Mostly in terms of extensibility, but I am not sure what our plan for that is yet, so we may never go that route. It would only be if we decide that 3PD can extend the form using PHP, which in turn would require some kind of config to define a field, which makes it easier if all fields have the same inputs/outputs.

That being the case, sounds good to me. Although this should definitely be something that we make the team aware of, since it would require consistency in order to be more valuable.

True, I will throw something up in Slack in the short term.

I wonder if we might want a conditional confirmation dialogue here if they've made changes, since the user will lose any attributes they've plugged in on accidental clicks.

Yeah that is a good suggestion, I can definitely add this. I could add a browser alert, similar to the one we show when deleting on of the attributes?
cc'ing @jarekmorawski on this.

I did add this btw as part of: 3f269cf
Let me know if the copy should change?

Instead of passing all input props here, it would be clearer within the <AttributeField /> component if we just move the useContext hook inside. Of course you could make the case that this is a bit more flexible, so entirely optional if we see other use cases that could benefit.

As mentioned in my reasoning for the value prop. My vision for these fields was similar as InputFields, where they are relatively stand alone but follow the same format as InputFields.
Moving the useContext hook inside makes it too reliant on the Form component incase we wanted to use it outside of that context, or if we decide to export it and a 3PD wants to use it outside of that context.

Yeah, I thought that may be the case. I would say that it's likely that an input field would normally appear inside of a form in some manner, but I understand the value of treating this as a standard input. Sounds good 👍🏻

A general thought on this component. It's currently accepting a value prop that is expected to be the format of an AttributeTerm as it's returned from the API, although the fetching itself is done in this component. Hence is someone wants to preselect certain values (cough edit modal ;)) then this is a bit of a catch-22.

I could potentially address this is my follow-up PR, but if you have any thoughts or want to address it here, that will definitely be a requirement for this component.

Hmmm this is actually a tough one from my perspective, as the ProductAttribute returns a list of terms as an array of strings by name (not slugs 😢 ), it would be a bit strange to change the entire field to only use a string array, as there could be duplicate names, and in general it's not specific enough.

An option could be to add an option to pass the product id in and retrieve all the terms by product id ( the getProductAttributeTerms api allows for this) and use that as the value instead, but then we have a slight dilemma on what the source of truth is, the value prop or the terms coming from the data store attached to the specific product id.

My proposal would be to leave this as is, and handle the retrieving of the ProductAttributeTerm outside of this field, in a similar manner as what I do now, I only set the options prop in the Modal not in the AttributeTerm field.

Retrieving all the product attributes for the edited product should be relatively easy by doing:

getProductAttributeTerms({
  atttribute_id: attributeId,
  product: productId
});

You could trigger that within the edit modal and pass those values straight into the ProductAttributeTermsInputField.
Hope that makes sense? and does my reasoning seem ok in this regard?

the ProductAttribute returns a list of terms as an array of strings by name (not slugs cry ),

By this, do you mean the ProductAttribute API? Definitely annoying that it wouldn't include the slugs 🤔

I'm fine with this approach, although it feels mildly painful to throw another request in. That makes sense given our limitations.

By this, do you mean the ProductAttribute API?

I actually mean the Products API, if you look at the Product - Attributes properties type here: https://woocommerce.github.io/woocommerce-rest-api-docs/#product-properties you can see it returns a list of options (an array of strings) which is a "list of available term names of the attribute.".

This would mean you did have to trigger a request anyhow to display the items correctly within the typeahead field. If names were unique this wouldn't be as much of a problem and we could probably rely on names only, but given that is not the case, we should definitely retrieve the full attribute term object using the attribute term API endpoint.