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

fixed->issue:147 search bar missing, reason:themes:[ Array ] in confi… #149

Closed
wants to merge 2 commits into from
Closed

Conversation

masterghost2002
Copy link
Contributor

fixed->issue:147 search bar missing, reason:themes:[ Array ] in config consisting of localsearch is over written by themes [Array] in extendedConfig

Explanation

All the changes are done in docusaurus.config.js

The reason for the search bar is not visible the, themes array [line no 62] in the config object which again consist a array of "@easyops-cn/docusaurus-search-local" the extension which is responsible for the search bar, is being overwritten by the array in extendedConfig object.

Related issue

fixes#147

What type of PR is this

/fixes
/feature-required

Proposed Changes

Merge the themes array present in the extendedConfig object which overwrites the themes array present in config object.

Live link for testing

Live Testing Link

ScreenShot

Screenshot from 2023-08-18 09-58-41

…g consisting of localsearch is over written by themes [Array] in extendedConfig
Copy link
Collaborator

alabulei1 commented Aug 18, 2023

Hello, I am a code review bot on flows.network. Here are my reviews of code commits in this PR.


Overall Summary:

The pull request aims to fix an issue where the search bar was missing. The patch involves adding a closing parenthesis to resolve a syntax error and adjusting the configuration related to the search feature.

Potential Issues and Errors:

  1. Syntax error: The patch introduces a missing semicolon and closing parenthesis on line 27.
  2. Unclear change in "themes": It is ambiguous whether the addition of a theme in the "themes" array is intentional or not. It is recommended to verify if the added theme is necessary and if it could potentially conflict with existing themes.

Important Findings:

  1. Separate commits: The pull request includes two separate commits. It is generally recommended to keep each pull request focused on a single change to facilitate easier review and testing.
  2. Insufficient commit message: The commit message lacks sufficient context about the root cause of the issue and how the patch resolves it.
  3. Lack of details: The patch does not provide information about the previous value of the "themes" array or the expected behavior.

In summary, the patch addresses the missing search bar issue, but it would be beneficial to address the potential issues and errors mentioned above. Additionally, the pull request could be improved by providing more context in the commit message and providing details about the previous value of the "themes" array.

Details

Commit 3ee4f71ab4bc3c44e0ef3232fda823d40338dfa3

Key changes:

  • The patch fixes an issue where the search bar was missing.
  • The previous configuration for the search feature was overwritten by themes in the extendedConfig.

Potential problems:

  • The patch introduces a syntax error. There is a missing semicolon at the end of line 27, where the closing parenthesis is missing.
  • It's unclear whether the change from themes: ['@docusaurus/theme-mermaid'] to themes: ['@docusaurus/theme-mermaid', ...] is intentional or not. It may be worth verifying if the added theme is necessary or if it conflicts with any existing themes.

Other findings:

  • The patch includes two separate commits. It's generally recommended to keep each pull request focused on a single change to facilitate easier code review and testing.
  • The commit message lacks sufficient context. It would be helpful to provide more details about the root cause of the issue and how the patch addresses it.

Commit f14c08e244c4bfdb417d2c6f752ea12f0d6d1f82

Key changes:

  • Fixed an issue with the search bar being missing.
  • The issue was caused by the "themes" array in the config overwriting the "localsearch" value in the "extendedConfig".

Potential problems:

  • The patch only includes a single change, which is the addition of a closing parenthesis on line 11. It's unclear if this is the intended change or if there are other changes missing from the patch.
  • The commit message is not clear and could be more descriptive.
  • The patch does not provide any details about the previous value of the "themes" array or the expected behavior.

Overall, the key change appears to fix the issue related to the search bar, but it would be beneficial to have more context and information about the changes made.

@mhmohona
Copy link
Contributor

Thank you for your work!
It seems your PR needs to be DOC passed.

…g consisting of localsearch is over written by themes [Array] in extendedConfig

Signed-off-by: Rakesh Dhariwal <[email protected]>
@masterghost2002
Copy link
Contributor Author

already did the signed off don't know why DCO failing, it will be helpful if you explain a bit more

@juntao
Copy link
Member

juntao commented Aug 19, 2023

already did the signed off don't know why DCO failing, it will be helpful if you explain a bit more

Your second commit does not have a DCO.

3ee4f71

You can click on "Details" next to the DCO check to see how to fix it. Thanks!

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