feat: configuration documentation #1953
Draft
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…to color_redesign
…to color_redesign
…to main_page_in_header
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @nitrosx - I've reviewed your changes - here's some feedback:
- The repeated ng-container blocks for each menu item could be refactored into a single *ngFor-driven template to reduce boilerplate and make adding/removing items easier.
- Instead of manually subscribing and unsubscribing to selectIsLoggedIn, consider deriving all login-based config values via the async pipe or RxJS combineLatest in the component or template to simplify the logic and avoid manual subscription management.
- You might centralize default config fallbacks (e.g. siteHeaderLogoUrl resolution and defaultMainPage selection) in the AppConfigService, keeping the component focused on rendering rather than default value logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The repeated ng-container blocks for each menu item could be refactored into a single *ngFor-driven template to reduce boilerplate and make adding/removing items easier.
- Instead of manually subscribing and unsubscribing to selectIsLoggedIn, consider deriving all login-based config values via the async pipe or RxJS combineLatest in the component or template to simplify the logic and avoid manual subscription management.
- You might centralize default config fallbacks (e.g. siteHeaderLogoUrl resolution and defaultMainPage selection) in the AppConfigService, keeping the component focused on rendering rather than default value logic.
## Individual Comments
### Comment 1
<location> `src/app/_layout/app-header/app-header.component.html:53` </location>
<code_context>
- <mat-icon> center_focus_weak</mat-icon>
- <span>Samples</span>
- </button>
+ <ng-container *ngIf="mainMenuConfig?.samples && false">
+ <button mat-menu-item routerLink="/samples/">
+ <mat-icon> center_focus_weak</mat-icon>
</code_context>
<issue_to_address>
The menu item for 'samples' is always hidden due to '&& false'.
If this is meant as a temporary feature toggle, please add a clarifying comment or use a configuration flag for better maintainability.
Suggested implementation:
```
<!-- Feature toggle: showSamples controls visibility of the Samples menu item -->
<ng-container *ngIf="mainMenuConfig?.samples && mainMenuConfig?.showSamples">
<button mat-menu-item routerLink="/samples/">
<mat-icon> center_focus_weak</mat-icon>
<span>Samples</span>
</button>
</ng-container>
```
To fully implement this, ensure that `showSamples` is defined in your `mainMenuConfig` object in the relevant TypeScript file (e.g., `app-header.component.ts` or its provider), and set to `true` or `false` as needed for the feature toggle.
</issue_to_address>
### Comment 2
<location> `docs/configuration/configuration.md:182` </location>
<code_context>
+ - Required
+### siteHeaderLogo:
+ - Type: string | null;
+ - Description: Name of the file with that contains the logo of the facility/lab/company/legal entity that runs the instance of scicat. This shuld be only the name of the file. The path is relative to _`src/assets/images`_.
+ If SciCat FE is run in a container using the official release image, the file should be mounted in the container under the path just mentioned.
+ - Example: assets/images/ess-logo-small.png
</code_context>
<issue_to_address>
Typo and awkward phrasing in the siteHeaderLogo description.
Reword to 'Name of the file that contains the logo...' and correct 'shuld' to 'should'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
- Description: Name of the file with that contains the logo of the facility/lab/company/legal entity that runs the instance of scicat. This shuld be only the name of the file. The path is relative to _`src/assets/images`_.
If SciCat FE is run in a container using the official release image, the file should be mounted in the container under the path just mentioned.
=======
- Description: Name of the file that contains the logo of the facility/lab/company/legal entity that runs the instance of scicat. This should be only the name of the file. The path is relative to _`src/assets/images`_.
If SciCat FE is run in a container using the official release image, the file should be mounted in the container under the path just mentioned.
>>>>>>> REPLACE
</suggested_fix>
### Comment 3
<location> `docs/configuration/configuration.md:189` </location>
<code_context>
+ - Required
+### siteHeaderLogoLink:
+ - Type: string;
+ - Description: This string will tell which url should be shown when th euser click on the side logo in the header. It can be an in-app path or a full URL.
+ - Examples:
+ If we want to show the datasets list when the user click on the site logo, the value should be set to _`/datasets`_.
</code_context>
<issue_to_address>
Typo in 'th euser' and awkward phrasing.
Correct 'th euser' to 'the user' and rephrase for clarity, e.g., 'This string specifies the URL to open when the user clicks the site logo in the header.'
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
- Description: This string will tell which url should be shown when th euser click on the side logo in the header. It can be an in-app path or a full URL.
=======
- Description: This string specifies the URL to open when the user clicks the site logo in the header. It can be an in-app path or a full URL.
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `docs/configuration/configuration.md:251` </location>
<code_context>
+ authenticatedUser: keyof typeof MainPageOptions;
+ }
+ ```
+ - Description: esplicitly configure which is the in-app page that the users are redirected to when visiting the main FE url. As you can see from the structure, sites can define different main pages for authenticated and non-authenticated users.
+ - Options: DATASETS, PROPOSALS, INSTRUMENTS, SAMPLES (as defined in enum `MainPagesOptions`)
+ - Default: DATASETS
</code_context>
<issue_to_address>
Typo in 'esplicitly'.
Please correct 'esplicitly' to 'explicitly'.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
- Description: esplicitly configure which is the in-app page that the users are redirected to when visiting the main FE url. As you can see from the structure, sites can define different main pages for authenticated and non-authenticated users.
=======
- Description: explicitly configure which is the in-app page that the users are redirected to when visiting the main FE url. As you can see from the structure, sites can define different main pages for authenticated and non-authenticated users.
>>>>>>> REPLACE
</suggested_fix>
### Comment 5
<location> `docs/configuration/configuration.md:276` </location>
<code_context>
+ }
+ ```
+ - Description:
+ Main menu configuration which controls which menu items are active. There are two configurations, one for authenticated and one for non-auhtenticated users. If not configured, the FE will show a `No menu items configured` element. Each menu item might have additional conditions to be visible.
+ - Example:
+ The following example will instruct the FE to show a menu with items datasets, instruments, proposals, and samples to non authenticated users. While for authenticated users, the menu will have all the items enabled, except for policies.
</code_context>
<issue_to_address>
Typo in 'non-auhtenticated'.
Please correct the spelling to 'non-authenticated'.
Suggested implementation:
```
Main menu configuration which controls which menu items are active. There are two configurations, one for authenticated and one for non-authenticated users. If not configured, the FE will show a `No menu items configured` element. Each menu item might have additional conditions to be visible.
```
```
The following example will instruct the FE to show a menu with items datasets, instruments, proposals, and samples to non-authenticated users. While for authenticated users, the menu will have all the items enabled, except for policies.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| <mat-icon> center_focus_weak</mat-icon> | ||
| <span>Samples</span> | ||
| </button> | ||
| <ng-container *ngIf="mainMenuConfig?.samples && false"> |
There was a problem hiding this comment.
suggestion: The menu item for 'samples' is always hidden due to '&& false'.
If this is meant as a temporary feature toggle, please add a clarifying comment or use a configuration flag for better maintainability.
Suggested implementation:
<!-- Feature toggle: showSamples controls visibility of the Samples menu item -->
<ng-container *ngIf="mainMenuConfig?.samples && mainMenuConfig?.showSamples">
<button mat-menu-item routerLink="/samples/">
<mat-icon> center_focus_weak</mat-icon>
<span>Samples</span>
</button>
</ng-container>
To fully implement this, ensure that showSamples is defined in your mainMenuConfig object in the relevant TypeScript file (e.g., app-header.component.ts or its provider), and set to true or false as needed for the feature toggle.
Comment on lines
+182
to
+183
| - Description: Name of the file with that contains the logo of the facility/lab/company/legal entity that runs the instance of scicat. This shuld be only the name of the file. The path is relative to _`src/assets/images`_. | ||
| If SciCat FE is run in a container using the official release image, the file should be mounted in the container under the path just mentioned. |
There was a problem hiding this comment.
issue (typo): Typo and awkward phrasing in the siteHeaderLogo description.
Reword to 'Name of the file that contains the logo...' and correct 'shuld' to 'should'.
Suggested change
| - Description: Name of the file with that contains the logo of the facility/lab/company/legal entity that runs the instance of scicat. This shuld be only the name of the file. The path is relative to _`src/assets/images`_. | |
| If SciCat FE is run in a container using the official release image, the file should be mounted in the container under the path just mentioned. | |
| - Description: Name of the file that contains the logo of the facility/lab/company/legal entity that runs the instance of scicat. This should be only the name of the file. The path is relative to _`src/assets/images`_. | |
| If SciCat FE is run in a container using the official release image, the file should be mounted in the container under the path just mentioned. |
| - Required | ||
| ### siteHeaderLogoLink: | ||
| - Type: string; | ||
| - Description: This string will tell which url should be shown when th euser click on the side logo in the header. It can be an in-app path or a full URL. |
There was a problem hiding this comment.
issue (typo): Typo in 'th euser' and awkward phrasing.
Correct 'th euser' to 'the user' and rephrase for clarity, e.g., 'This string specifies the URL to open when the user clicks the site logo in the header.'
Suggested change
| - Description: This string will tell which url should be shown when th euser click on the side logo in the header. It can be an in-app path or a full URL. | |
| - Description: This string specifies the URL to open when the user clicks the site logo in the header. It can be an in-app path or a full URL. |
| authenticatedUser: keyof typeof MainPageOptions; | ||
| } | ||
| ``` | ||
| - Description: esplicitly configure which is the in-app page that the users are redirected to when visiting the main FE url. As you can see from the structure, sites can define different main pages for authenticated and non-authenticated users. |
There was a problem hiding this comment.
issue (typo): Typo in 'esplicitly'.
Please correct 'esplicitly' to 'explicitly'.
Suggested change
| - Description: esplicitly configure which is the in-app page that the users are redirected to when visiting the main FE url. As you can see from the structure, sites can define different main pages for authenticated and non-authenticated users. | |
| - Description: explicitly configure which is the in-app page that the users are redirected to when visiting the main FE url. As you can see from the structure, sites can define different main pages for authenticated and non-authenticated users. |
| } | ||
| ``` | ||
| - Description: | ||
| Main menu configuration which controls which menu items are active. There are two configurations, one for authenticated and one for non-auhtenticated users. If not configured, the FE will show a `No menu items configured` element. Each menu item might have additional conditions to be visible. |
There was a problem hiding this comment.
issue (typo): Typo in 'non-auhtenticated'.
Please correct the spelling to 'non-authenticated'.
Suggested implementation:
Main menu configuration which controls which menu items are active. There are two configurations, one for authenticated and one for non-authenticated users. If not configured, the FE will show a `No menu items configured` element. Each menu item might have additional conditions to be visible.
The following example will instruct the FE to show a menu with items datasets, instruments, proposals, and samples to non-authenticated users. While for authenticated users, the menu will have all the items enabled, except for policies.
Comment on lines
+99
to
+101
| this.siteHeaderLogoUrl = this.config.siteHeaderLogoUrl | ||
| ? this.config.siteHeaderLogoUrl | ||
| : this.defaultMainPage; |
There was a problem hiding this comment.
suggestion (code-quality): Avoid unneeded ternary statements (simplify-ternary)
Suggested change
| this.siteHeaderLogoUrl = this.config.siteHeaderLogoUrl | |
| ? this.config.siteHeaderLogoUrl | |
| : this.defaultMainPage; | |
| this.siteHeaderLogoUrl = this.config.siteHeaderLogoUrl || this.defaultMainPage; | |
Explanation
It is possible to simplify certain ternary statements into either use of an|| or !.This makes the code easier to read, since there is no conditional logic.
Junjiequan
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
This PR introduce the first version of a comprehensive list of all the configuration options available.
The first options that will be described are the ones related to the header restyle PR
Motivation
While I was getting the header restyle PR, I realized that the FE configuration contains many options and there is no description for them.
Changes:
Please provide a list of the changes implemented by this PR
Tests included
Documentation
official documentation info
If you have updated the official documentation, please provide PR # and URL of the pages where the updates are included
Backend version
Summary by Sourcery
Introduce comprehensive frontend configuration documentation and extend header component to leverage configuration options for menu items, branding, and default landing pages, along with related UI refinements and test updates.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: