-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Add support for controlling entity settings visibility during runtime #7816
base: master
Are you sure you want to change the base?
Conversation
Signed-off-by: Juho Heikka <[email protected]>
@@ -36,6 +39,7 @@ export interface RegisteredEntitySetting { | |||
components: EntitySettingComponents; | |||
source?: string; | |||
group: string; | |||
isShown: IComputedValue<boolean>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not call this also visible
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to replicate the existing behavior that was implemented with AppPreferenceTabRegistration
. I think it's a convention in this code base for Showable
and such
@@ -36,6 +39,7 @@ export interface RegisteredEntitySetting { | |||
components: EntitySettingComponents; | |||
source?: string; | |||
group: string; | |||
isShown: IComputedValue<boolean>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this should be optional to not break something..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The EntitySettingRegistration
from extension has optional visible
. When the extension settings are read the extension settings are converted to RegisteredEntitySetting
and then we can make this not optional which is nicer for the users of RegisteredEntitySetting
. We default to visible then if the visible
property is not set
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we get a unit test showing that this implementation is working please?
Also this is an enhancement, it should not go against a patch release @jweak |
Okay I'm not that familiar how the milestones work. There's a high prio bug in an extension that needs this though. |
oh okay I see, sure |
Signed-off-by: Juho Heikka <[email protected]>
Good point! Added unit tests. |
Extension might want to control visibility of entity settings during runtime. This change adds this ability. Has the same logic than earlier implementation for
AppPreferenceTabRegistration
Resolves #7815