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

Module property initialization #219

Merged
merged 10 commits into from
May 20, 2021

Conversation

fschmenger
Copy link
Collaborator

The purpose of this change-set is to forward the complete module configuration found in app-conf.json for each module. The configuration can then be picked up within each module via Vue props, rather than having to access the global $appConfig.modules.

The idea behind is:

  • Provide better encapsulation of module configurations.
  • Make it easier to validate properties (by using the implicit mechanism provided by Vue).
  • Easily support the new properties introduced by the base Module cards - which now can all be configured via the config-
  • Simplify the module initialization code.
  • Revive some old properties like e.g. 'title', that we're not passed through from the app-config.json into the modules.

@fschmenger
Copy link
Collaborator Author

This change only affects the properties declared in the 'modules' array of app-conf.json.

  • Some modules, e.g. Maps use global fields from app-conf.json to populate their configuration data.
  • AppTemplate and AppHeader also access the global $appConfig object.

We could take the idea of this change-set a step further and consider to access the $appConfig only at top level program initialization. From there we could then rely on properties to pass the configuration through the component tree. IMO this would be nice, however requires a bit more work.

@fschmenger
Copy link
Collaborator Author

Another little refactoring I propose would be to move the 'darkLayout' property from the individual modules and make it a global setting. Since the hamburger menu now shares the same background color as the toolbar and the window headers, there should be no drawbacks. This probably could be reviewed in conjunction with #202.

Copy link
Collaborator

@chrismayer chrismayer left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @fschmenger. This is an important step towards the encapsulation of modules as a pre-step for having a Wegue core library in the future.
Big thanks for also considering backwards compatibility again.

@chrismayer
Copy link
Collaborator

Also here gain for transparency: There will be some more PRs tackling some refactoring tasks. The last step will be to adjust the unit tests at once to avoid duplicate work, so it is a know issue that this (and some follow ups) will break the test suite of the master branch for a couple days.

@chrismayer chrismayer merged commit 1b64034 into wegue-oss:master May 20, 2021
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