Skip to content

Move option resolution to helpers.options#7125

Closed
kurkle wants to merge 1 commit intochartjs:masterfrom
kurkle:options
Closed

Move option resolution to helpers.options#7125
kurkle wants to merge 1 commit intochartjs:masterfrom
kurkle:options

Conversation

@kurkle
Copy link
Member

@kurkle kurkle commented Feb 19, 2020

  • enforces consistent option fallback
  • encourages consistent option naming (steppedLine = stepped or lineStepped (with type prefix))
  • makes all options scriptable (most are already though)
  • makes all options to be considered with 'hover' prefix in active mode
  • makes all options prefixable by element type
    • borderColor -> arcBorderColor
    • hoverBorderColor -> arcHoverBorderColor
  • enabled all element options to be specified in chart.options (because of spanGaps)
    • this needs to be discussed, but feels like it could make Chart.js simpler to use
  • simplifies resolution customization in controller level
  • makes adding new options to an element a bliss (only need to change the element)
  • reduces size by 2259 bytes
this pr: 175 646 bytes
master: 177 905 bytes

downsides

  • reduces control from dataset controllers (personally I do not consider this a downside)

@benmccann
Copy link
Contributor

I didn't review super closely yet. I think it'll be a bit easier to take a look after rebase since a lot of the changes are already in

@kurkle
Copy link
Member Author

kurkle commented Feb 20, 2020

Thanks for taking a look. I'll change this to "ready for review", when all the non related stuff is merged and rebased, so don't waste too much time on it yet :)

@kurkle kurkle force-pushed the options branch 5 times, most recently from 2538d75 to 0df8468 Compare February 21, 2020 20:16
@kurkle kurkle force-pushed the options branch 2 times, most recently from 932d246 to 130946f Compare February 22, 2020 17:55
Copy link
Member

@etimberg etimberg left a comment

Choose a reason for hiding this comment

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

Not sure this is the best way to go, but there are some good changes inside of this that I think are worth merging such as moving to static _defaults on the element classes.

@kurkle kurkle changed the title Move option resolution to core.element Move option resolution to helpers.options Feb 23, 2020
@kurkle kurkle force-pushed the options branch 2 times, most recently from b342c24 to 7e8cc92 Compare February 23, 2020 13:05
@benmccann
Copy link
Contributor

This is a pretty big one. Perhaps it could be broken into multiple PRs?

@kurkle
Copy link
Member Author

kurkle commented Feb 23, 2020

#7135 is from this. I'm kind of holding back, because it seems the older ones become stale if there are many PR's.

@benmccann
Copy link
Contributor

Rebase this one now that #7135 has been merged?

@kurkle
Copy link
Member Author

kurkle commented Feb 28, 2020

rebased

@kurkle kurkle marked this pull request as ready for review February 28, 2020 17:46
@kurkle
Copy link
Member Author

kurkle commented Feb 29, 2020

Probably not going to get merged, so I'll just close this.

@kurkle kurkle closed this Feb 29, 2020
const indexable = (key, index) => notIndexable.indexOf(key) !== -1 ? undefined : index;

/**
* @param {*} elementType
Copy link
Contributor

Choose a reason for hiding this comment

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

This function is used enough in core code that it could be helpful to add descriptions to these parameters

@benmccann
Copy link
Contributor

benmccann commented Mar 1, 2020

A couple notes of things I noticed:

  • I'm not sure making the dataElementType static really is much of a win because you then have to do me.constructor.dataElementType whenever you want to access it instead of me.dataElementType
  • I'm not sure whether putting the options in the elements really makes sense. There are a lot of options, especially in the bar controller, like categoryPercentage that really only make sense in the context of the controller. If you put this option on a rectangle, I'm not sure that makes as much sense because it's really not specific to a rectangle. Some of the elements like point are used in multiple controllers, so putting controller options in the elements could become confusing in that case - though I don't think that's particularly an issue in the codebase it could be for custom code our users have written
  • We had discussed the possibility of removing the element options. I think that might be something worth doing first. That would probably remove the duplication between the element and controller options

@kurkle kurkle deleted the options branch June 12, 2020 05:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants