rename eventCallbacs to events #26

Closed
opened 2 months ago by glitch4347 · 5 comments
Owner

This is definition of Options

export type Options = {
    margin?: Margin;
    eventsCallbacks?: {
    ....

I think better name just "events" like everywhere in js

@vargburz what do you think?

This is definition of Options ``` export type Options = { margin?: Margin; eventsCallbacks?: { .... ``` I think better name just "events" like everywhere in js @vargburz what do you think?
Owner

agree, events sounds better

agree, events sounds better
Poster
Owner

I shoudl add that eventsCallbacks is obsolete code and keep it for backward compatibility

I shoudl add that eventsCallbacks is obsolete code and keep it for backward compatibility
glitch4347 changed title from rename evenctCallbacs to events to rename eventCallbacs to events 2 months ago
Poster
Owner

Doing this I found even deeper bad code.

I belive that we don't need CoreOptions

export class CoreOptions<O extends Options> {
  _options: O;

  constructor(options: O, private _podDefaults?: Partial<O>) {
    this.setOptions(options);
  }

  public updateOptions(options: O): void {
    this.setOptions(options);
  }
...

and transform

export type Options = {
  margin?: Margin;

this to class

because in index.ts we do

  constructor(
    protected readonly el: HTMLElement,
    _series: T[] = [],
    _options: O
  ) {
    // TODO: test if it's necessary
    styles.use();

    this.options = new CoreOptions(_options);
    this.series = new CoreSeries(_series);

anyway

Doing this I found even deeper bad code. I belive that we don't need CoreOptions ``` export class CoreOptions<O extends Options> { _options: O; constructor(options: O, private _podDefaults?: Partial<O>) { this.setOptions(options); } public updateOptions(options: O): void { this.setOptions(options); } ... ``` and transform ``` export type Options = { margin?: Margin; ``` this to class because in index.ts we do ``` constructor( protected readonly el: HTMLElement, _series: T[] = [], _options: O ) { // TODO: test if it's necessary styles.use(); this.options = new CoreOptions(_options); this.series = new CoreSeries(_series); ``` anyway
Poster
Owner

I think that CoreOptions should be renamed to Optinos,
when original Options to OptionsConfig

I think that `CoreOptions` should be renamed to `Optinos`, when original `Options` to `OptionsConfig`
Poster
Owner

and remove generic for o extends Options because client code should inherit CoreOptions and resolve it's config.

and remove generic for `o extends Options` because client code should inherit `CoreOptions` and resolve it's config.
glitch4347 closed this issue 2 months ago
Sign in to join this conversation.
No Label
No Milestone
No project
No Assignees
2 Participants
Notifications
Due Date

No due date set.

Dependencies

This issue currently doesn't have any dependencies.

Loading…
There is no content yet.