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

DateRangeFilter crashes if the current locale isn't supported #346

Closed
diomidov opened this issue Nov 28, 2019 · 3 comments · Fixed by #347
Closed

DateRangeFilter crashes if the current locale isn't supported #346

diomidov opened this issue Nov 28, 2019 · 3 comments · Fixed by #347
Labels

Comments

@diomidov
Copy link

I'm submitting a Bug report

Your Environment

Software Version(s)
Angular 7.2.15
Angular-Slickgrid 2.12.3
TypeScript 3.1.6
flatpickr 4.6.3
Operating System Windows 10

Context

My current locale is ua. I want to create a column with a date range filter. Flatpickr doesn't support ua locale out of the box, so I tried explicitly specifying a supported locale:

filter: {
  model: Filters.dateRange,
  filterOptions: {
    locale: 'en',
  } as FlatpickrOption,
},

Expected Behavior

A date range filter that uses English locale

Current Behavior

Error: "Cannot find module './ua.js'"
    webpackContextResolve .*\.js$:73
    webpackContext .*\.js$:67
    Angular 6
        loadFlatpickrLocale
        buildDatePickerInput
        createDomElement
        init
        addFilterTemplateToHeaderRow
        bindLocalOnFilter
    notify slick.core.js:183
    trigger slick.grid.js:2555
    createColumnHeaders slick.grid.js:1303
    finishInitialization slick.grid.js:557
    Angular 37
        initialization
        ngAfterViewInit
        ...

(sorry, this doesn't show typescript line numbers)

Possible Solution

Here is the relevant part of DateRangeFilter.buildDatePickerInput:

  //...
  let currentLocale = this.translate && this.translate.currentLang || 'en'; // returns 'ua'
  //...
  const pickerOptions = {
    //...
    // tries to read ua.js and throws an exception because ua.js doesn't exist
    locale: (currentLocale !== 'en') ? this.loadFlatpickrLocale(currentLocale) : 'en',
    //...
  }
  //...
  // override the current locale with the locale from filterOptions
  this._flatpickrOptions = { ...pickerOptions, ...(this.columnFilter.filterOptions as FlatpickrOption) };
  //...

I think loadFlatpickrLocale should return en if it can't load the locale, instead of throwing an exception. Alternatively you can avoid calling this.loadFlatpickrLocale(currentLocale) if pickerOptions.locale exists.

Code Sample

@ghiscoding ghiscoding added the bug label Nov 28, 2019
ghiscoding pushed a commit that referenced this issue Nov 28, 2019
- fixes #346
- when using a locale that Flatpickr doesn't support, like "ua", it should use "en" (English) as default locale.
  - it will display a console warning that this locale is not supported but not throw an error (unless user set a different locale, see next)
  - user can choose to override the locale by setting locale to use via `filterOptions: { locale: 'en' }`
@ghiscoding
Copy link
Owner

ghiscoding commented Nov 28, 2019

That is a valid bug, I created a PR #347 to address wath you wrote. However please note that I will show a console warning (see the print screen in the PR) to advise user that it's not a supported locale in Flatpickr, I could have use English straight away without any console warnings, but I think it's better to advise the user (you) that it's not a valid Flatpickr locale and so you know where the error originated. If you read the warning, I mentioned that you can avoid seeing the console warning by overriding the locale via the filterOptions, which is what you wanted.

BTW, there's 2 console warnings in the Filter by Range Example since there are 2 different Date Filters (which both had to be fixed, those are the compoundDate and dateRange filters). The warning shows exactly which filter it originated from.

See the PR #347 for more info and if you could confirm that this is the behavior you wanted, I'll merge it right after.

Thanks for reporting and suggesting fixes.
Cheers ⭐️

@ghiscoding
Copy link
Owner

I'll merge the PR and go with that since I didn't feedback from your side and I need to fix another bug and do another release soon.

@ghiscoding
Copy link
Owner

ghiscoding commented Nov 29, 2019

Released under version 2.14.3

Also if you haven't already, please upvote ⭐️
Cheers

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants