Why is this needed? Can we just fallback to the type's Edit function (which is already a user input) when the "type" is defined.

We introduced userInput prop because we want to differentiate the user input from the preset options. Previously, the filter can only be added if field has element. But for integer filer, users should be able to enable filter without adding any elements.

Enabling the filter based on the field type is an option, but it might not provide enough flexibility. For the integer type, I think these scenarios should be supported:

1. No filter needed for the field
2. Filter with preset options (set elements)
3. Filter with user input (userInput: true)
4. Filter with preset options and user input (not sure if this is needed?)

Does this make sense? Or we should always enable the filter for integer type?

For me if there's no elements, it should just fallback automatically to userInput, so the prop might not be needed at all.

I've left some comment below on the API that is worth checking, but iterating on it and addressing the states we want to support:

1. No filter needed for the field.

{
  id: 'id', 
  type: 'integer'
}

2. Filter with preset options and user input.

This is the default experience for typed fields that provide elements. It's the same for date.

{
  id: 'id', 
  type: 'integer',
  elements: [ ... ]
}

3. Filter with only user input.

{
  id: 'id',
  type: 'integer',
  filterBy: {
    input: true
  }
}

4. Filter with only presets

{
  id: 'id',
  type: 'integer',
  elements: [ ... ],
  filterBy: {
    input: false
  }
}

Thanks for the feedback! @youknowriad @oandregal

For this PR, I'll go with the approach that @oandregal suggested.

• if field.type is integer, display the Edit control for the filter (input control for integers, by default, but the consumer can provide its own).
• if field.elements is provided, display presets as well.

For Woo Analytics, the design requires support for hiding the input control. However, I'll double check with the designer. If the designer still prefers hiding the input control, I'll work on it in a separate PR.

Not sure I like the "input" config personally, I'd rather start opinionated: by default "input", if there are elements "presets", no mix of both, and potentially have a way to disable a field from being a filter.

I'd rather start opinionated: by default "input", if there are elements "presets", no mix of both, and potentially have a way to disable a field from being a filter.

This sounds good to me. We don't have use cases for both "input" and "presets" at the same time.

Quick summary:

We don't have use cases for both "input" and "presets" at the same time.

Yes, we do :) The date filter is one use case, for example. This is the default experience in my view.

{ type: 'integer' }

I don't think this field configuration should render any filter. Filter have been "opt-in" so far, and I think it makes more sense for them to stay that way. To opt-in, either you provide elements or you enable something so it displays the Edit/input. Additionally, consumers should be able to disable the input/Edit or the presets.

Based on the use cases you mentioned before, I'll elaborate a bit more on how I see this working:

• By default, a field doesn't provide any filter:

{ type: 'integer' } 

• If it has elements, it provides a filter with input/Edit and presets (this is the default experience):

{ type: 'integer', elements: [ ... ] }

• It should be possible to disable the input or the presets:

{ type: 'integer', elements: [ ... ], filterBy: { widget: [ 'presets' ] } }  // only enable presets
{ type: 'integer', elements: [ ... ], filterBy: { widget: [ 'edit' ] } }  // only enable edit

• It should be possible to enable the filter even if the field doesn't have elements:

{ type: 'integer', filterBy: { widget: [ 'edit' ] } } 

I care about supporting this experiences, but I don't mind much about using filterBy.input, filterBy.widget or any other naming. I'll be AKF next week, so I won't be able to provide feedback, but wanted to share how I expect this to work.

Yes, we do :) The date filter is one use case, for example. This is the default experience in my view.

Ah, you're right! The date filter does require both input and presets. 🤔

I care about supporting this experiences, but I don't mind much about using filterBy.input, filterBy.widget or any other naming.

I don't mind the naming either. I think the key point is whether the filter should be enabled or disabled by default.

Since we want to support input and presets, I'd agree to disable the filter by default to avoid clutter and confusion; no filter is rendered unless the consumer explicitly opts in.

Why is this checking for a new prop userInput? Can we use the field type instead? For context, that's what I've explored in the date filter.

We use userInput prop because if users only provide preset options, we should render SearchWidget. We can't use the field type to determine if we should render UserInputWidget or SearchWidget.

#103276 (comment)

We use userInput prop because if users only provide preset options, we should render SearchWidget

This is how I'd expect it to work:

• if field.type is integer, display the Edit control for the filter (input control for integers, by default, but the consumer can provide its own).
• if field.elements is provided, display presets as well.

Do you need support for the case of hiding the input control? Elements without input is how the current filters work (untyped fields). If you want to add that in this PR, I'd suggest a generic approach that works for any field type later. For example, adding a new flag under filterBy to only render presets (the default experience is using everything):

{
  id: '...',
  type: 'integer',
  elements: [ ... ]
  filterBy: {
    presetsOnly: true
  }
}

Additionally, I think the SearchWidget should just have different widgets per field type:

if ( props.filter.type === 'datetime' ) { ... }
if ( props.filter.type === 'integer' ) { ... }

// Default widgets that just render elements.

As we iterate on this, using this approach would be clearer and has the following advantage: when/if we allow 3rd parties to define their own field types, they'll just work if we're able to make the filter part of the field definition. It's good to have it in mind, but it's not a requirement to implement it like that right now.

Continued the conversation above #103276 (comment)

I'm not sure I understand what's happening here and why we're creating this new "InputWidget" component, I believe we should just rely on the "edit" defined by the field type.

The InputWidget component is created for managing the onChange event and the filter value. It does rely on the Edit component within it.

Additionally, the InputWidget component will accommodate operators such as between and not between (we
plan to add these in another PR) for integer types, which require the rendering of two Edit components.

Makes sense, but we shouldn't be hard coding the "type", we should check the presence of "edit" and the config of the "type" instead. It's important to understand that when we add a new "type" to dataviews, we shouldn't have to touch any other file in the library, registration of types should be decoupled from the framework itself.

Okay, thanks for the clarification. 🙏

I thought we wanted to add support for the integer field first, and gradually add support for other field types. The type logic is temporary and will be removed in the future. 🤔

Makes sense, but we shouldn't be hard coding the "type", we should check the presence of "edit" and the config of the "type" instead.

Then, this PR will affect all field types. Any fields with an Edit Prop will enable filter support. cc @oandregal

The NormalizeField appears to guarantee the presence of the Edit prop. In this case, we should verify that the Edit returns non-null elements, right?

The NormalizeField appears to guarantee the presence of the Edit prop. In this case, we should verify that the Edit returns non-null elements, right?

This is interesting, you're right, it does seem like it's always defined. Personally, I think we should change the normalized field to have a null Edit when there's no edit defined (basically when the field has no "type" or the field has no explicit "Edit" function).

I thought we wanted to add support for the integer field first, and gradually add support for other field types.

You're right, making this change will enable the direct input for all types, I think it's a good thing though.

Makes sense, but we shouldn't be hard coding the "type", we should check the presence of "edit" and the config of the "type" instead. It's important to understand that when we add a new "type" to dataviews, we shouldn't have to touch any other file in the library, registration of types should be decoupled from the framework itself.

As I mentioned, I agree with this direction, but I am fine if we start limiting by type for now (making it work based on the Edit requires a few big changes, so starting with the types helps contain scope). I'm also fine if we want to make it all work for all types at once in a single PR.

Thanks, I've updated the PR as suggested.

• Changed Edit Prop type to accept null
• Added user input filter support based on the Edit property
• Added default filterBy operators for each field type
• Updated PR description and changelog to reflect the changes

This PR doesn't add a ability to disable the filter yet. I'll add that in a separate PR. Please review when you have time and let me know if anything else needs to be changed. 🙏

This Edit function doesn't have access to the whole object, just one value. This is not a restriction we have in DataForm, where the Edit is used. What if the Edit access values other than the field.id (e.g., "price" + "discount" to generate a "consolidated price")?

I've changed how InputWidget passes data to Edit, providing all filter values instead in 5920eed.

The idea of NormalizedFilter (or NormalizedField) is that all props should exist, so code that deals with this type doesn't need to do checks for existance (we're normalized them before). In this case, why does it need to change to undefined? I haven't checked all the code, but I suppose we're using this to check something. Can't we check for size of the array?

I changed it to optional because the useFilters doesn't normalize the elements prop, it's pass through as is so it could be undefined or an empty array. If that's the case, then it's a bug. I'll revert the type and fix the bug to ensure the elements are always defined. We can check the size.

Fixed filter.elements type and checked array size instead. 58f3f5e

Same thought here about "NormalizedField".

Additionally, if the filter is opt-in, as suggested, do we need to change Edit? We'd check for the presence of elements of a filterBy config instead of Edit, no?

Modifying the Edit function could prevent situations where the filter is opt-in, yet the Edit function displays empty content. Certain type definitions within the Edit function can lead to this issue. I'll check the code and think deeper about this.

I consider FieldDefinition as "good defaults" for each type. Instead of allowing it to be undefined, can we making the filterBy.operators mandatory for every field type (and also for the untyped fallback)? It's okay if the fieldtype definition says "no operator" (empty set).

Sure, I'll make the operators mandatory.

Updated filterBy property to be required and set with default operators in field types in 09072f3.

I feel the name of this component is a bit weird, this seems to be the actual filter (not a summary)

Indeed, it can be confusing. This is the code where the component is used. Renaming it to 'Filter' would be clearer. I'll rename it.

Renamed FilterSummary component to Filter in fe6fada.

Needs an empty line

Fixed in 53f987c.

@chihsuan @youknowriad I want to make sure everyone is aware that every 2 week we sync changes from Gutenberg upstream into the packages/dataviews folder in calypso as per PCYsg-173c-p2.

Renaming a file is the kind of thing we shouldn't do. Most of the time it'll go fine, but it can create unnecessary work in these syncs — for example, if anyone creates a new file filter-summary.tsx, git won't be able to detect the original summary was renamed. My recommendation is that we revert this file rename.

I'm also not super stoked about other renames in this PR (FilterSummary to Filter, etc.) for the same reasons, but their surface for errors is minor — it's okay to keep them if you feel strongly about them. My general recommendation would be to make the minimal change that works and avoid needless refactors (renames, reorganizations, etc.).

This may sound like a little thing, but if we're not diligent with the kind of changes we make, we'll run into issues with the sync process.