feat(@mongodb-js/compass-query-bar): Add prop to toggle visibility of sorts square bracket placeholders CLOUDP-97063

[chrisrichie25]

Description

This PR adds the ability for one to hide the square sorts place holder in the sort option of the query bar. This feature makes it possible for one to be clear about the type of sorts that are supported in the query bar. For example, in the case that the square brackets are not supported, one can hide the sort square brackets placeholder.

Checklist

• New tests and/or benchmarks are included
• Documentation is changed or added

Motivation and Context

• Bugfix
• New feature
• Dependency update
• Misc

Open Questions

Dependents

Types of changes

• Backport Needed
• Patch (non-breaking change which fixes an issue)
• Minor (non-breaking change which adds functionality)
• Major (fix or feature that would cause existing functionality to change)

[gribnoysup]

This should not be changed manually, seems like something might've gone wrong during the merge/rebase?

[chrisrichie25]

Yap, seems like that is what happened.

[gribnoysup]

Left a comment suggesting a different approach for the change you're planning to introduce. It would be also interesting to learn what environment doesn't support arrays as sort option, it seems to be supported by the node driver for some time now

[gribnoysup]

I don't think it's the approach to allow for the behavior you want, this api feels very specific and hard to scale (what if next you would want to change a label on another field, how many of those custom props for any of them will end up in this component interface?). Maybe something more generic that treats OPTION_DEFINITION as default options and allows to override them with props might be more appropriate here

[chrisrichie25]

Got it! Will fix this and push again

[gribnoysup]

Thanks for addressing the feedback! The new implementation is definitely much better, but I still have a note about it. If you feel like chatting about it, definitely feel free to reach out.

I would also still encourage you to check with the team why this change is needed as array sort option should be a supported way of filtering and more or less the only one that would allow to get an expected sort result when object keys are number-like (e.g., {"3": 1, "2": 1, "1": -1})

[gribnoysup]

We're totally getting close here, that's a much nicer implementation, but in a way have the same issue the previous implementation had, it only allows to override the placeholder for one specific filed which is not really intuitive and that's the main problem I see here.

If we are changing this part of the code, I think we might as well allow those placeholder overrides for all fields. Check out how the value and validation status are derived for the fields a few lines above:

compass/packages/compass-query-bar/src/components/query-bar/query-bar.jsx

Lines 209 to 217 in 04db91c

	// for filter only, also validate feature flag directives
	const hasError = option === 'filter'
	? !(filterValid || featureFlag)
	: !this.props[`${option}Valid`];
	// checkbox options use the value directly, text inputs use the
	// `<option>String` prop.
	const value = OPTION_DEFINITION[option].type === 'boolean' ?
	this.props[option] : this.props[`${option}String`];

I think we might want to do something similar here for consistency, for example:

Suggested change

	const placeholder = option === 'sort' && sortOptionPlaceholder ? sortOptionPlaceholder : OPTION_DEFINITION[option].placeholder;
	const placeholder = this.props[`${option}Placeholder`] || OPTION_DEFINITION[option].placeholder;

[chrisrichie25]

Got it!

[gribnoysup]

Great work, thanks!

[Anemy]

nice