-
Notifications
You must be signed in to change notification settings - Fork 15
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
Update index.ts #35
base: master
Are you sure you want to change the base?
Update index.ts #35
Conversation
fix TS errors
Ty for writing this module. Could you please accept @hracik changes for multiple classes on button and these TS error fixes and then release an updated version? Unfortunately, at the moment the latest version 0.3.1 has no 'class' on supportedAttributes. |
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.
Left a small nitpick, other than that I don't see why the other changes are needed as I'm not getting any typescript errors; @sh1AD1ded mind elaborating on why/if those are actually needed?
Thanks for your contribution 👍
@@ -88,7 +88,7 @@ export function ShortcutButtonsPlugin(config: ShortcutButtonsFlatpickr.Config) { | |||
return; | |||
} | |||
|
|||
const index = parseInt(target.dataset.index, 10); | |||
const index = parseInt(target.dataset.index as string, 10); |
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.
@jcsmorais index could be undefined, that's why I get TS error -->
@@ -119,7 +119,7 @@ export function ShortcutButtonsPlugin(config: ShortcutButtonsFlatpickr.Config) { | |||
/** | |||
* Set given button's attributes. | |||
*/ | |||
function setButtonsAttributes(button: HTMLButtonElement, attributes?: ShortcutButtonsFlatpickr.Attributes) { | |||
function setButtonsAttributes(button: HTMLButtonElement, attributes: ShortcutButtonsFlatpickr.Attributes) { |
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.
@jcsmorais your are checking for undefined before calling this function, no need for attributes to be optional
@@ -136,7 +136,7 @@ export function ShortcutButtonsPlugin(config: ShortcutButtonsFlatpickr.Config) { | |||
*/ | |||
onReady: () => { | |||
wrapper = document.createElement('div'); | |||
wrapper.classList.add('shortcut-buttons-flatpickr-wrapper', cfg.theme); | |||
wrapper.classList.add('shortcut-buttons-flatpickr-wrapper', cfg.theme as string); |
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.
@jcsmorais theme is defined as string | undefined (gives TS error) but you're always setting a default theme, therefore it's never undefined at this point, simpliest way is to use "as string"
@@ -176,6 +176,7 @@ export function ShortcutButtonsPlugin(config: ShortcutButtonsFlatpickr.Config) { | |||
* Clean up before flatpickr is destroyed. | |||
*/ | |||
onDestroy: () => { | |||
if (typeof wrapper === 'undefined') return; |
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.
@jcsmorais changed this alike your suggestion
@@ -74,7 +74,7 @@ export function ShortcutButtonsPlugin(config: ShortcutButtonsFlatpickr.Config) { | |||
/** | |||
* Element that wraps this plugin's dependent elements. | |||
*/ | |||
let wrapper: HTMLElement; | |||
let wrapper: HTMLElement | undefined; |
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.
@jcsmorais can be undefined bc its set to undefined onDestroy(), I get TS errors if it's not defined that way
+1 because
|
@jcsmorais should I check in something? |
fix TS errors