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.

Sign up for GitHub

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

Jump to bottom

Add the ability to override the TopMenu component on the MainLayout via IOC #472

Merged
merged 2 commits into from
Oct 10, 2023
Merged

Add the ability to override the TopMenu component on the MainLayout via IOC #472

merged 2 commits into from
Oct 10, 2023

Conversation

Robbware
Copy link
Contributor

@Robbware Robbware commented Oct 9, 2023

Prerequisites

  • I have written a descriptive pull-request title
  • I have verified that there are no overlapping pull-requests open
  • I have verified that I am following the COMET-WEB code style guidelines
  • I have provided test coverage for my change (where applicable)

Description

This will allow us to override the TopMenu component that is rendered on the MainLayout.

@Robbware Robbware added the enhancement New feature or request label Oct 9, 2023
@Robbware Robbware self-assigned this Oct 9, 2023
@Robbware
Copy link
Contributor Author

Robbware commented Oct 9, 2023 •
edited
Loading

My other idea would be to move the components that we load on the body of the TopMenu (ApplicationMenu, ModelMenu, SessionMenu) to the RegisteredAuthorizedMenuEntries that we register on the IOC. This would make it so anything that depends on this app will not have these loaded by default on the top header. What do you think @antoineatrhea ?

Edit: waiting on feedback before fixing up the code coverage.

@antoineatstariongroup
Copy link
Contributor

antoineatstariongroup commented Oct 9, 2023 •
edited
Loading

My other idea would be to move the components that we load on the body of the TopMenu (ApplicationMenu, ModelMenu, SessionMenu) to the RegisteredAuthorizedMenuEntries that we register on the IOC. This would make it so anything that depends on this app will not have these loaded by default on the top header. What do you think @antoineatrhea ?

Edit: waiting on feedback before fixing up the code coverage.

@Robbware fine for me

@Robbware
Add the ability to override the TopMenu component on the MainLayout
742e1f4 
Revert "Add the ability to override the TopMenu component on the MainLayout"

This reverts commit 46c3043.

Use IOC to load all Items within the TopMenu
@Robbware Robbware force-pushed the Fix/allow-override-of-topmenutype-on-mainlayout branch from 46c3043 to 742e1f4 Compare October 9, 2023 15:47
@sonarqubecloud
Copy link

sonarqubecloud bot commented Oct 9, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

warning The version of Java (11.0.20.1) you have used to run this analysis is deprecated and we will stop accepting it soon. Please update to at least Java 17.
Read more here

antoineatstariongroup
antoineatstariongroup approved these changes Oct 10, 2023
View reviewed changes
COMET.Web.Common/Services/RegistrationService/IRegistrationService.cs Outdated
@@ -59,5 +59,7 @@ public interface IRegistrationService
/// Gets the <see cref="Type" /> to use as MainLayout for the application
/// </summary>
Type MainLayoutType { get; }

Type TopMenuType { get; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing documentation

@Robbware Robbware merged commit db5f137 into development Oct 10, 2023
7 checks passed
@Robbware Robbware deleted the Fix/allow-override-of-topmenutype-on-mainlayout branch October 10, 2023 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@antoineatstariongroup antoineatstariongroup antoineatstariongroup approved these changes

Assignees

@Robbware Robbware

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Robbware @antoineatstariongroup