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

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

Conversation

rapsealk
Copy link
Contributor

@rapsealk rapsealk commented Nov 7, 2023

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 ...

  • New feature
  • Bug fix
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Refactoring
  • Code style optimization
  • Test Case
  • README update
  • Other (about what?)

🔗 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. ⚠️

  • Demo in storybook is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • Tests are updated and passed without a decrease in coverage
  • Format (lint & prettier) script passed
  • Build script is working
  • README API section is updated or not needed

@xrutayisire xrutayisire self-requested a review November 7, 2023 10:26
@xrutayisire xrutayisire added the bug Something isn't working label Nov 7, 2023
Copy link

codecov bot commented Nov 7, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

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

Additional details and impacted files
@@           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 Coverage Δ
src/components/CustomSelect.tsx 73.25% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@xrutayisire
Copy link
Owner

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
Copy link
Contributor Author

rapsealk commented Nov 7, 2023

@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
Copy link
Owner

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
Copy link
Contributor Author

rapsealk commented Nov 7, 2023

@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
Copy link
Owner

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

@xrutayisire xrutayisire merged commit 3ea350e into xrutayisire:master Nov 7, 2023
5 checks passed
@rapsealk rapsealk deleted the fix/enable-filter-option-for-weekdays-and-months branch November 7, 2023 14:16
@rapsealk
Copy link
Contributor Author

rapsealk commented Nov 8, 2023

@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
Copy link
Owner

Sorry, I couldn't find the time. I will do it now and yes it's currently manually.
Here is the test I did: 95af1fa

@xrutayisire
Copy link
Owner

You can use v5.0.1

@rapsealk
Copy link
Contributor Author

rapsealk commented Nov 8, 2023

@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
Copy link
Owner

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
Copy link
Owner

Thanks for everything and happy for new PRs 🙂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants