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

FR: Provide a way to configure inputs at the module level #62

Closed
1 of 2 tasks
rafaelss95 opened this issue Feb 22, 2021 · 5 comments · Fixed by #77
Closed
1 of 2 tasks

FR: Provide a way to configure inputs at the module level #62

rafaelss95 opened this issue Feb 22, 2021 · 5 comments · Fixed by #77

Comments

@rafaelss95
Copy link

I'm submitting a ... (check one with "x")

  • bug report => search github for a similar issue or PR before submitting
  • feature request

Current behavior
Currently we can't set a custom loadingText globally. I've proposed it before in #56 (comment).


Expected behavior
A way to pass a default config to the .forRoot(). It'd be really nice to set a custom loadingText globally, and why not the other properties as well.

@NgModule({
  NgxSkeletonLoader.forRoot({ animation: false, loadingText: 'My custom text' }),
})

or maybe an InjectionToken where we can configure it even at the component-level.


What is the motivation / use case for changing the behavior?
A non-repetitive of way of defining inputs like loadingText.

Please tell us about your environment:

  • Browser: [all | Chrome XX | Firefox XX | IE XX | Safari XX | Mobile Chrome XX |
    Android X.X Web Browser | iOS XX Safari | iOS XX UIWebView | iOS XX WKWebView ]

  • Language: [all | TypeScript X.X | ES6/7 | ES5]
    Angular 11, ngx-skeleton-loader 2.9.0.

@rafaelss95 rafaelss95 mentioned this issue Feb 22, 2021
5 tasks
@willmendesneto
Copy link
Owner

@rafaelss95 Yeah, that makes sense to me. Thanks for the feature request, you're welcome to create a PR.

@HunteRoi
Copy link
Contributor

HunteRoi commented Jun 1, 2021

This seems like a reasonable request. I'm willing to participate. Although I don't quite know how this could be implemented as the parameters are actually component's attributes with @Input.

Anyone to guide and review once the PR is done ?

@willmendesneto
Copy link
Owner

Hi @HunteRoi. Thanks for help on that, mate!

One approach that might be used here is by creating a service and adding it to be used on service constructor. E.G. https://angular.io/guide/dependency-injection

So we can have a service like NgxSkeletonLoaderServiceConfig when we are adding all the information added via forRoot

NgxSkeletonLoaderModule.forRoot({
  loadingText: 'Please wait, content is loading...',
  animation: 'pulse',
})

And adding it on NgxSkeletonLoaderComponent, checking and overriding in case there's no default value being passed to the component.

So, based on this rule and example, we can assume:

  • If there's no animation value, we should set animation with pulse value as default
  • If there's animation value, the given value should be used since that means it was added intentionally.

Happy to help on the review or in any other way I can, mate!

@HunteRoi
Copy link
Contributor

HunteRoi commented Jun 1, 2021

Is it worth creating types ? eg

// types
export type Appearance = 'circle' | 'line' | '';

export type Animation = 'progress' | 'progress-dark' | 'pulse' | 'false' | false;

export type Theme = {
  // This is required since ngStyle is using `any` as well
  // More details in https://angular.io/api/common/NgStyle
  // tslint:disable-next-line: no-any
  [k: string]: any;
};

export class NgxSkeletonLoaderConfig {
  appearance: Appearance = 'line';
  animation: Animation = 'progress';
  theme: Theme = {};
  loadingText: string = 'Loading...';
  count: number = 1;
}

export const defaultConfig = new NgxSkeletonLoaderConfig();

//service
@Injectable({ providedIn: 'root' })
export class NgxSkeletonLoaderConfigService {
  readonly config: NgxSkeletonLoaderConfig = defaultConfig;

  constructor(@Optional() config: NgxSkeletonLoaderConfig) {
    if (config) this.config = config;
  }
}

This would mean that the changes on the library are more important though.

@willmendesneto
Copy link
Owner

If NgxSkeletonLoaderConfig is something used in a single spot, perhaps there's no need of it. We can live on NgxSkeletonLoaderConfigService for better maintenance. Also, there's no need to have NgxSkeletonLoaderConfig as a class since a const can do the trick.

const defaultConfig =  {
  appearance: 'line',
  animation: 'progress',
  theme: {},
  loadingText: 'Loading...',
  count: 1,
} as NgxSkeletonLoaderConfig;
// ...

For the types, I would say we can apply that in a different matter, but the idea is exactly that!

export type NgxSkeletonLoaderConfig = {
  appearance: 'circle' | '';
  animation: 'progress' | 'progress-dark' | 'pulse' | 'false' | false;
  theme: {
    // This is required since ngStyle is using `any` as well
    // More details in https://angular.io/api/common/NgStyle
    // tslint:disable-next-line: no-any
    [k: string]: any;
  };
};

And, it can reuse it on the component itself. E.G.

export class NgxSkeletonLoaderComponent implements OnInit, AfterViewInit, OnDestroy, OnChanges {
  // ...
  @Input()
  appearance: NgxSkeletonLoaderConfig['appearance'] = '';
  // ...
}

Don't worry if some of the things I mentioned at the moment are not totally clear. Also, I would encourage you to raise a pull request with the changes, thoughts and suggestions. So we can have some even better discussions there.

PS: Nice initiative @HunteRoi

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 a pull request may close this issue.

3 participants