-
Notifications
You must be signed in to change notification settings - Fork 4.6k
DataViews: update operator labels and deprecate the isNotAll
#73671
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
Conversation
|
Size Change: +129 B (+0.01%) Total Size: 2.58 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 3b4a59b. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/19824597828
|
3b4a59b to
c831a42
Compare
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
isNotAll operator and update labelsisNotAll
| }; | ||
|
|
||
| if ( filterInView?.operator === OPERATOR_IS_ANY ) { | ||
| if ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I guess we can return early if filterInView?.operator === undefined and not part of the supported operators.
Additionally the createInterpolateElement is the same in both cases with just the active element(s) label, so we could have a single call based on single selection operators. And with that I'm wondering if we could use SINGLE_SELECTION_OPERATORS and ALL_OPERATORS instead of importing them one by one. I'm not sure though if SINGLE_SELECTION_OPERATORS is updated because here you have more like OPERATOR_BETWEEN.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually by looking at the removed code for between , OPERATOR_IN_THE_PAST etc, we had different handling, which was just removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored at 4a94005
packages/dataviews/src/constants.ts
Outdated
| label: __( 'Is not' ), | ||
| }, | ||
| [ OPERATOR_IS_ANY ]: { | ||
| key: 'is-any-filter', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not from this PR, but why would we need the key prop? I also couldn't find any usage for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not in the original scope of this PR, but I refactored how operators are registered for simplicity at 4a94005
packages/dataviews/src/constants.ts
Outdated
| [ OPERATOR_IS_ANY ]: { | ||
| key: 'is-any-filter', | ||
| label: __( 'Is any' ), | ||
| label: __( 'Includes' ), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if all these labels should have a translators note to clarify where they are used and what the quantifiers ("any", "none", "all") refer to. From a quick look at GlotPress, I see the current translations for es_ES, which are grammatically inconsistent:
| English | Spanish |
|---|---|
| Is none | No es ninguno |
| Is any | Es cualquiera |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed at f68740c
3f826a6 to
4a94005
Compare
| sprintf( | ||
| /* translators: 1: Filter name. 2: Filter value. e.g.: "Author is not all: Admin, Editor". */ | ||
| __( '<Name>%1$s is not all: </Name><Value>%2$s</Value>' ), | ||
| /* translators: 1: Filter name. 2: Operator name. 3: Filter value. e.g.: "Author is any: Admin, Editor". */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We call it "operator name", but it forms a verbal group in the string ("is any"). I found this slightly confusing, so I suspect translators will too.
| /* translators: 1: Filter name. 2: Operator name. 3: Filter value. e.g.: "Author is any: Admin, Editor". */ | ||
| __( '<Name>%1$s %2$s: </Name><Value>%3$s</Value>' ), | ||
| filter.name, | ||
| operator.display ?? operator.label.toLowerCase(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For me this is a smell that we are doing too much with .display. I know that repetition is annoying, but this is probably better for translators:
{
name: OPERATOR_IS_NONE,
labels: {
/* translators: DataViews operator name */
name: __( 'Is none of' ),
/* translators: DataViews filter label, e.g. "Author is none of:" */
filterText: __( '%s is none of:' ),
},
selection: 'multi',
}Don't take my naming choices literally, use your best intuition; the point is the separation of strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misread the code initially, so ignore a part of my feedback. But:
- We shouldn't resort to
.toLowerCase, which porbably means making.displaymandatory. - It seems safer to move the interpolation into the
.displaystring (%s is none of:instead ofis none of). I even considered going further and moving the entire thing into the hands of translators:<Name>... </Value>, but maybe it's too much and more error-prone.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I extracted the operators to a separate utility that collocates the operator names and the text string it generates at c9d65ee
370898e to
c9d65ee
Compare
| const shouldResetValue = | ||
| currentOperator && | ||
| ( OPERATORS_SHOULD_RESET_VALUE.includes( | ||
| const currentOpSelectionModel = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hard-coding the operator names here, I've made it filter agnostic. It works as before, but it now depends on how the filter works with the data (single value selection, multi value selection, or custom value).
For example, number can switch between the regular single value (number) to a custom format (between operator):
Screen.Recording.2025-12-09.at.18.27.06.mov
And dates can switch between the regular single value (date) to a custom format (in the past, over):
Screen.Recording.2025-12-09.at.18.27.28.mov
|
@ntsekouras @mcsf is there anything else you'd like me to address on this one? It diverged quite a bit from the original purpose (update some text strings). It's for the best, because the operator code is now much better organized, though I'd like to prevent its scope from getting much bigger. |
mcsf
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't want to block you any further, we can make changes later if we want to!
I'm still on the fence about operator={ __( 'is' ) } versus making the interpolation more explicit for translators (%s is: %s), but we'll iterate if need be. (I also wish we had proper i18n experts, in whose absence I've been sharing opinionated takes based on what little i18n I know and on my polyglotism, but always unsure of myself...)
Co-authored-by: Miguel Fonseca <[email protected]>
Co-authored-by: Miguel Fonseca <[email protected]>
8dbee79 to
deaeaf2
Compare
I've done that at deaeaf2 The changes here improve over what we have in trunk, and they also address the feature (change filter text). This is not public API, so I'm inclined to land these fast and iterate if needed be. |
Thanks! |
Fixes #67124
isNotAlloperator because it's the same asisNone.