-
Notifications
You must be signed in to change notification settings - Fork 49
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
Support for optional counter types for ordered list #129
base: main
Are you sure you want to change the base?
Support for optional counter types for ordered list #129
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it seems, that user could specify empty array (or array with one value) for counterTypes
, then Counter type option should not be displayed in tool settings (since it would never be a choice)
you can add this changes and then it would be ready to merge, thanks for contributing!
Hi @e11sy, sorry I was busy this past few weeks. I've updated the PR. Feel free to review again. thank you |
src/index.ts
Outdated
/** | ||
* Dont show Counter type tune if there is no valid counter types | ||
*/ | ||
if (orderedListCountersTunes.children.items!.length > 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we will have 1 counter type, than it would be information plate, not a tune, better to add tunes, when user could change smth
if (orderedListCountersTunes.children.items!.length > 0) { | |
if (orderedListCountersTunes.children.items!.length > 1) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah it make sense. kindly review the PR again thanks @e11sy
src/index.ts
Outdated
/** | ||
* Set the default counter types for the ordered list | ||
*/ | ||
this.defaultCounterTypes = this.config?.counterTypes || ['numeric', 'upper-roman', 'lower-roman', 'upper-alpha', 'lower-alpha']; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we use typeof Object.values of the OlCounterTypesMap to show, where it all goes from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with that since we have the values already in the map. I pushed a change in this line.
several small changes and feature would be done, thanks for improvements |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for delay in reviewing, now it seems fine, thanks for contributing
can you update version to then feature could be merged |
The current version has a fixed list of counter types, which are the default options for ordered list.
So this PR allows an optional way to specify a list on what counter types should be visible.
Example
Closes #128