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

#4303: add search function. #4304

Merged
merged 5 commits into from
Jun 4, 2024
Merged

Conversation

zyhfish
Copy link
Contributor

@zyhfish zyhfish commented Jun 3, 2024

#4303: add search function.

@sbwalker
Copy link
Member

sbwalker commented Jun 3, 2024

@zyhfish this is a very large PR and will take some time to review. Here is some initial feedback:

General

I do not prefer the term “Document” … I prefer “Item” or “Content” ie. SearchContent

In general, we do not create global constants for specific Settings unless we expect the setting to be used in many places throughout the framework (ie. the goal is to keep the global constants to a minimum).

Client project

Search Results module - will be accessible to anonymous users - should be a statically rendered module component - not interactive.

core services should be organized the same way as other core services - just because the other modules are in the Admin folder/namespace does not mean they are only intended for Admin users - it means that they are part of the "core" framework. The only exception is the Html/Text module which is an intended to be an example of how a third party module would be organized.
Components should be in Modules/Admin/SearchResults
Services should be in /Services folder
Etc…

Themes/Controls/Theme - is where Search.razor component should be located (where all of the other theme components are located)

Server project

DefaultSiteTemplate - adds the new search page for new sites. I would prefer if the Url was simply "/search?q=" rather than /search-results
in order to add a page to existing sites it needs to be added to Oqtane.Server\Infrastructure\UpgradeManager.cs for the specific version - see Upgrade_3_0_1

on the server, the core services should be organized the same way as other core services ie. /Controllers is where the core Controllers belong, etc... not in a subfolder under /Modules (the HtmlText module is the exception and should not be used as an example).

Shared project

Since other module interfaces use the “able” naming convention (IPortable, IInstallable, etc..) it would make sense for IModuleSearch to be named ISearchable.

@zyhfish
Copy link
Contributor Author

zyhfish commented Jun 4, 2024

Hi @sbwalker , thanks so much for the review, the code has been refactored with the suggestions, could you please help to check again? thx.

@sbwalker
Copy link
Member

sbwalker commented Jun 4, 2024

@zyhfish merging this PR now... additional refactoring can be done through other PRs

@sbwalker sbwalker merged commit d449396 into oqtane:dev Jun 4, 2024
1 check passed
@zyhfish zyhfish deleted the task/add-search-function branch June 5, 2024 02:12
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