Skip to content
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

Change disabling of search autocomplete #2886

Merged
merged 1 commit into from
Jan 10, 2025
Merged

Conversation

csutter
Copy link
Contributor

@csutter csutter commented Jan 9, 2025

This was previously managed by rendering a different component in every single rendering application (due to the autocomplete functionality being present in the global header).

This changes it to be a single feature flag on search-api-v2 instead that controls whether any results are returned, and changes it to be a positive flag instead.

This was previously managed by rendering a different component in
_every_ single rendering application (due to the autocomplete
functionality being present in the global header).

This changes it to be a single feature flag on `search-api-v2` instead
that controls whether any results are returned, and changes it to be a
positive flag instead.
@csutter csutter marked this pull request as ready for review January 9, 2025 15:13
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but would suggest we probably don't want the superfluous ENABLE_AUTOCORRECT env vars and just want the instructions to tell someone to add it.

@csutter
Copy link
Contributor Author

csutter commented Jan 10, 2025

Looks good, but would suggest we probably don't want the superfluous ENABLE_AUTOCORRECT env vars and just want the instructions to tell someone to add it.

You're right that the app sets a default for the environment variable, so it's not strictly necessary.

But I think it would still be helpful to specify it in the app config, for two reasons:

  • as long as the feature flag exists, it makes it more explicit that we want it enabled (especially now that it's a positively named flag)
  • it makes it easier to find for someone who urgently needs to turn off autocomplete but is maybe unfamiliar with the app and/or less confident with our Helm set up

Happy to remove it though if you feel strongly about it.

Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nah, not strong opinions on it. Your points are valid so happy to keep it.

@csutter csutter merged commit cb7bfb8 into main Jan 10, 2025
4 checks passed
@csutter csutter deleted the change-search-ac-flag branch January 10, 2025 14:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants