Skip to content

Conversation

@razonyang
Copy link
Contributor

@razonyang razonyang commented May 7, 2023

This PR exposes a template (empty by default) for customizing the search input, which will be included if all existing search options were disabled.

The purpose of this PR is that avoid overriding the layouts/partials/search-input.html which will make other search options unusable and hard to maintain on users/clients side.

Copy link
Contributor

@deining deining left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

@chalin chalin force-pushed the custom-search-input branch from e55ea7b to e48d3b7 Compare May 31, 2023 14:43
Copy link
Collaborator

@chalin chalin left a comment

Choose a reason for hiding this comment

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

  • See inline comment.
  • Also, this new feature should be mentioned in the docs.

@chalin chalin added this to the 23Q3 milestone May 31, 2023
@razonyang razonyang force-pushed the custom-search-input branch from e48d3b7 to 4b4210e Compare May 31, 2023 15:19
@razonyang razonyang force-pushed the custom-search-input branch from 4b4210e to a0df1b8 Compare May 31, 2023 15:20
@chalin chalin modified the milestones: 23Q3, 23Q4 Jul 27, 2023
@chalin chalin modified the milestones: 23Q4, 24Q1 Nov 3, 2023
@chalin chalin modified the milestones: 24Q1, 24Q2 Jan 11, 2024
@chalin chalin modified the milestones: 24Q2, 24Q3 Apr 2, 2024
@chalin chalin modified the milestones: 24Q3, 24Q4 Jul 2, 2024
@chalin chalin modified the milestones: 24Q4, 25Q1 Oct 3, 2024
@chalin chalin modified the milestones: 25Q1, 25Q2 Jan 7, 2025
@SayakMukhopadhyay
Copy link
Contributor

I recently raised #2194 which is related. I think I would prefer if there was a guard for the custom search implementation instead of relying on the presence of search-input-custom.html to activate the feature. All search features are activated by guards and I think having a customSearch or custom-search parameter as a guard would be more explicit and transparent.

It will also help downstream users to use the same parameter as a check while implementing other stuff in other layouts. Moreover, as mentioned in the issue, I would prefer to also override the search script in scripts.html using the same guard.

We can include the script in our search-input-custom.html partial but I prefer to include the script in scripts.html to take the benefit of minification and a single script bundle. See the related PR kubernetes/website#49724 for an idea.

@chalin
Copy link
Collaborator

chalin commented Mar 13, 2025

@SayakMukhopadhyay et all: Thinking about the proposed solution in this PR, why not simply override layouts/partials/search-input.html, which is what the custom partial would be doing.

@SayakMukhopadhyay
Copy link
Contributor

@SayakMukhopadhyay et all: Thinking about the proposed solution in this PR, why not simply override layouts/partials/search-input.html, which is what the custom partial would be doing.

@chalin That is indeed how things are implemented in the k8s website right now. In general, I have a personal preference for hooks over overriding. Hooks make it easier to upgrade. Docsy has some opinions on how layouts and especially CSS work with each other and keeping track of the changes while upgrading can become difficult, especially when the layouts are overridden.

I have found that while upgrading if I have overridden a Docsy layout, I have to put a lot of thought into how that layout interacts with other layouts (which can be multiple and not too easy to find at times) and then change the overridden layout to match according. This is especially true for the Docsy 0.6 to 0.7 migration that we have to do. I do this by comparing the 0.6 and 0.7 layouts with the overridden layouts and then figuring out the changes that needed to be made. Having a hook means that the hooked layout can be self-contained to a degree and the changes in the layout calling the hook would not affect the hooked layout.

At the same time, I understand that it's not feasible to add hooks everywhere but maybe it's possible in a few common places.

@chalin
Copy link
Collaborator

chalin commented Mar 18, 2025

Closing. For a statement of reasoning behind this, see #2194 (comment).

@chalin chalin closed this Mar 18, 2025
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.

4 participants