Skip to content

Feat/config #1969

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

Merged
merged 5 commits into from
Apr 12, 2022
Merged

Feat/config #1969

merged 5 commits into from
Apr 12, 2022

Conversation

ethanshar
Copy link
Collaborator

Description

The idea of the config class is to allow users set initial configurations that we'll take into account when initialization various components and classes
For example: whether to use platform colors for design tokens

I considered a few options to implement it:

  • Support some sort of a config file (like babel and eslint) that the user can create and we can read - the problem with this solution is that our project is not a Node project (like babel/eslint) and it can read files using fs API
  • [This solution] I created a (very) simple config class that the users can use to load their configurations, for this solution I had to create a standalone package for the config file so the user can load it independently without loading other uilib classes (colors, schemes) and give the users the chance to set the configurations before anything is using it.

Changelog

Support a config file for setting initial configuration for uilib

@M-i-k-e-l M-i-k-e-l self-requested a review April 12, 2022 07:50
@M-i-k-e-l M-i-k-e-l self-assigned this Apr 12, 2022
Comment on lines 8 to 17
class Config {
usePlatformColors = false;

public setConfig(options: ConfigOptions) {
const {usePlatformColors = false} = options;
this.usePlatformColors = usePlatformColors;
}
}

export default new Config();
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't like that the default value is set twice, maybe we can do something like this:

private usePlatformColors;
public setConfig(options: ConfigOptions) {
  const {usePlatformColors = false} = options;
  this.usePlatformColors = usePlatformColors;
}

constructor() {
  setConfig({}); // sets the default values
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed

@ethanshar ethanshar enabled auto-merge (squash) April 12, 2022 08:14
@ethanshar ethanshar merged commit d2f544e into master Apr 12, 2022
lidord-wix pushed a commit that referenced this pull request Apr 17, 2022
* Create Config class

* Create a standalone package for the config file

* Use config in schemes class

* Add description

* Avoid declaring default config values twice
@ethanshar ethanshar deleted the feat/config branch June 13, 2022 08:17
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