xrutayisire / react-js-cron

A React cron editor built with antd
https://xrutayisire.github.io/react-js-cron/?path=/docs/reactjs-cron--demo
MIT License
238 stars 105 forks source link

fix: Apply filterOption to custom select even with options list #64

Closed rapsealk closed 10 months ago

rapsealk commented 10 months ago

Hello react-js-cron!

First of all, thanks for maintaining the project.

I was looking for a way to limit week-days; for example, I want to disallow users to select Saturday and Sunday from week-days custom selector. I found that filterOption has been added since v4.1.0 and tried to use it. It works fine with other types except for week-days and months, which pass additional argument optionsList to <CustomSelect />. And I finally figured out that .filter(filterOption) is not called when optionsList exists.

πŸ€” This is a ...

πŸ”— Related issue link

I haven't reported an issue.

πŸ’‘ Background and solution

Try this with and without the PR:

<Cron
  ...
  dropdownsConfig={{
    "week-days": {
      filterOption: ({ value }) => Number(value) < 3,
    },
  }}
/>

β˜‘οΈ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

codecov[bot] commented 10 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Comparison is base (c3e021a) 87.00% compared to head (a785652) 87.00%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #64 +/- ## ======================================= Coverage 87.00% 87.00% ======================================= Files 12 12 Lines 531 531 Branches 234 234 ======================================= Hits 462 462 Misses 11 11 Partials 58 58 ``` | [Files](https://app.codecov.io/gh/xrutayisire/react-js-cron/pull/64?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Xavier+Rutayisire) | Coverage Ξ” | | |---|---|---| | [src/components/CustomSelect.tsx](https://app.codecov.io/gh/xrutayisire/react-js-cron/pull/64?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=Xavier+Rutayisire#diff-c3JjL2NvbXBvbmVudHMvQ3VzdG9tU2VsZWN0LnRzeA==) | `73.25% <100.00%> (ΓΈ)` | |

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

xrutayisire commented 10 months ago

Hi,

Nice catch! Thank you very much for this NICE fix! πŸ™ Can you fix the small prettier issue so I can merge?

Also, if you want to quickly add a test for what you did it's perfect, if not I could do it in the future πŸ˜‰

rapsealk commented 10 months ago

@xrutayisire Thanks for the comment! I've just applied prettier and pushed. I'm afraid that I'm almost new to writing test code for frontend, but I will try based on Cron.updateValue.test.tsx. See you in an hour.

xrutayisire commented 10 months ago

Exactly you should have some guidance, if you don't succeed, no problem, I will add some after and you could check them. Thank you!

rapsealk commented 10 months ago

@xrutayisire I was trying to check that there is only from "Thursday"~"Wednesday"~ to "Saturday" but no "Monday", but I failed. Would you mind if I skip for this time and add test code on next PR?

The code is below:

import { render, screen, waitFor } from '@testing-library/react';
import userEvent from '@testing-library/user-event';

import Cron from '../Cron';
import { CronType, DropdownsConfig, PeriodType } from '../types';

const WEEK_DAY_WEDNESDAY = 3
const MONTH_OCTOBER = 9

describe('Cron filter week-days', () => {
  it('should ', async () => {
    const user = userEvent.setup()
    const value = '* * * * *'
    const setValue = jest.fn()

    const allowedPeriods: PeriodType[] = ['minute', 'month', 'year']
    const allowedDropdowns: CronType[] = ['period', 'week-days']

    const dropdownsConfig: DropdownsConfig = {
      "week-days": {
        filterOption: ({ value }) => Number(value) > WEEK_DAY_WEDNESDAY,
      },
      // "months": {
      //   filterOption: ({ value }) => Number(value) > MONTH_OCTOBER,
      // },
    }

    render(
      <Cron
        value={value}
        setValue={setValue}
        // allowedPeriods={allowedPeriods}
        allowedDropdowns={allowedDropdowns}
        dropdownsConfig={dropdownsConfig}
      />
    )

    // Open Period dropdown
    await waitFor(() => {
      user.click(screen.getByText('minute'))
    })

    // Select year period
    await waitFor(() => {
      user.click(screen.getByText('year'))
    })

    // await waitFor(() => {
    //   expect(screen.getByTestId('select-period').textContent).toContain('year')
    // })

    // Open week days dropdown
    await waitFor(() => {
      user.click(screen.getByTestId('custom-select-week-days'))
      // user.click(screen.getByText('every day of the week'))
    })

    // Check dropdowns value
    await waitFor(() => {
      // expect(screen.findByText('Monday')).not.toBeInTheDocument()
      // expect(screen.findByText('FRI')).toBeNull()
      user.click(screen.getByText('Friday'))
    })
  })
})
xrutayisire commented 10 months ago

No problem, I will try to add some tests in master soon. I will release tonight your fix. Thank you again!

rapsealk commented 10 months ago

@xrutayisire May I ask a question? It seems that there is no GHA workflow related to deployment in this repository. Are you deploying new releases manually or in other way?

xrutayisire commented 10 months ago

Sorry, I couldn't find the time. I will do it now and yes it's currently manually. Here is the test I did: https://github.com/xrutayisire/react-js-cron/commit/95af1fa79e9f78a468f29e1b5178f9b1c8bae6a3

xrutayisire commented 10 months ago

You can use v5.0.1

rapsealk commented 10 months ago

@xrutayisire Sorry, I was not meaning to rush you, but thanks for the quick release! Do you have any plan to add a GHA workflow for automated release? There are many useful actions such as marvinpinto/action-automatic-releases. If you don't mind, I will make a new PR for this when I have time. Thanks again!

xrutayisire commented 10 months ago

Not really planned. I did some setup like that in the past. This release process is pretty simple so I never did it, but yeah why not in the future πŸ™‚ No sure about the necessary to use an external use like that tought. Release is so simple for this lib that it may be overkill.

xrutayisire commented 10 months ago

Thanks for everything and happy for new PRs πŸ™‚