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

Use ModuleMessage component everywhere #750

Merged
merged 6 commits into from
Oct 1, 2020

Conversation

hishamco
Copy link
Contributor

No description provided.

@hishamco hishamco marked this pull request as ready for review September 17, 2020 12:43
@hishamco
Copy link
Contributor Author

@sbwalker I just refactor the File Manager to you use the proposed alert component, if sound good to you I can refactor all the places to use the new component

@sbwalker
Copy link
Member

A component already exists for this functionality - the ModuleMessage component. It is used throughout the UI already- usually by calling the AddModuleMessage helper method - but also as a standard component.

@hishamco
Copy link
Contributor Author

I didn't know about it, let me check ...

@hishamco hishamco changed the title Add Alert component Use ModuleMessage component everywhere Sep 20, 2020
@hishamco
Copy link
Contributor Author

@sbwalker it's ready from my side, just one question why you create a Get and Set methods on the component while you can get or set parameters directly from the component instance?!!

@sbwalker
Copy link
Member

@hishamco the SetModuleMessage() method is only intended to be used in one specific case - when a module developer calls the AddModuleMessage() helper which then needs to traverse to the ModuleInstance parent component and set the Message/Type properties on its ModuleMessage component. There may be a better way to implement this. Regardless, in all other cases the ModuleMessage component should be used in the standard way - by setting the parameter values directly and not using the SetModuleMessage() method. Make sense?

@hishamco
Copy link
Contributor Author

by setting the parameter values directly and not using the SetModuleMessage() method. Make sense

I did that at the beginning but I got some warnings when I set the parameters directly, let me change it in all the places

@hishamco
Copy link
Contributor Author

Done!!

@sbwalker sbwalker merged commit 5f56bc2 into oqtane:master Oct 1, 2020
@hishamco hishamco deleted the alert-component branch October 1, 2020 14:10
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.

2 participants