Settings Fix #1021
Settings Fix #1021
Conversation
| moduleType.GetMethod("UpdateSettings")?.Invoke(_settings, null); // method must be public in settings component | ||
| } | ||
| } | ||
| _settings?.GetType().GetMethod("UpdateSettings")?.Invoke(_settings, null); // method must be public in settings component |
There was a problem hiding this comment.
We have to introduce interface for settings form (ISettingForm or ISettingControl),
I have now created one setup form and have had trouble remembering how it works.
|
This was caused by PR #968 - the way that Settings components are passed via ModuleState was changed which resulted in a regression issue. The proper fix should be: If you need to see an example of how this works you can look at the Blog example module: https://github.com/oqtane/oqtane.blogs/blob/master/Client/Settings.razor Basically the framework uses a convention that if a module has a component named "Settings" then it will dynamically load that component as a tab within the Manage Settings UI. The benefit of this approach is that the developer has full flexibility over the layout and UI interactions of the component - they are not restricted in any way in what they can do within the component. That being said, in order to be consistent with the user experience in other areas of the framework, there is only a single Save button in the Manage Settings UI ( rather than a Save button in each tab ). When a user selects the Save button the system needs to update all of the settings regardless of which tab they are displayed on. The way this is currently implemented is that a module Settings component must implement a public method: However this is not based on an actual interface - so it is not as intuitive from a developer perspective, as there is no "contract". So I do see the benefit of your suggestion of having an ISettingsControl interface which contains an UpdateSettings() method to make this contract more explicit. The question is if it is truly worth it to introduce the interface or not as the current method would still need to be preserved for backward compatibility - so it would result in 2 methods which would need to be supported/maintained in the long term. Could this be more simply improved in other ways - ie. better documentation, examples, etc.. |
|
Your fix code is in another notation, but result is same. I will do interface implementation with compatibility fallback tomorow, it is about add one if statement to current code. |
ISettingControl introduction
PavelVsl
force-pushed
the
UpdateSettings
branch
from
cfc7f33 to
91c5ff7
Compare
|
Bug fixed, |
|
Do we want to implement this in the Module Creator templates so that developers use the new method rather than the old method? |
|
yes, it's only about introduce interface in Settings.razor @implements Oqtane.Interfaces.ISettingsControl |
Settings of module is not saved because UpdateSettings method cannot be invoked