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

DatePicker: Write tests and refactor #194

Open
luizbaldi opened this issue Oct 6, 2020 · 5 comments
Open

DatePicker: Write tests and refactor #194

luizbaldi opened this issue Oct 6, 2020 · 5 comments
Assignees
Labels
good first issue Good for newcomers

Comments

@luizbaldi
Copy link
Member

The DatePicker component is already created and it's functional, but it was written some time ago and it can be refactored (migrating from class-based to hooks). In order to achieve a good result, we also need a good test coverage.

datepicker

Running storybook locally

  1. Fork and clone the repo
  2. Install the necessary dependêncies using npm (run npm i on the root folder)
  3. Start storybook locally by running npm start, this will start a local server and opens a tab on your browser automatically
  4. Uncomment the code on src/DatePicker/DatePicker.stories.js file in order to make it appear
  5. Create a DatePicker.spec.js file on the same folder to write tests

Aditional information

We're using React Testing Library to unit test our components and we have a good test coverage, so you can rely on the existing ones.


Before start working on this bug leave a comment here so we can avoid more than one person working on the issue, if you have any doubts feel free to drop a comment here or join us at Slack 🕺

@luizbaldi luizbaldi added the good first issue Good for newcomers label Oct 6, 2020
@luizbaldi luizbaldi changed the title Write tests and refactor (optional) DatePicker component Write tests and refactor DatePicker component Oct 6, 2020
@luizbaldi luizbaldi changed the title Write tests and refactor DatePicker component DatePicker: Write tests and refactor Oct 6, 2020
@luckened
Copy link

luckened commented Oct 6, 2020

hi! can i work on this?

@luizbaldi
Copy link
Member Author

@luckened For sure! Feel free to drop questions here if you need anything 🚀

@luckened
Copy link

@luizbaldi please take a look 🔍

@arturbien
Copy link
Member

@luckened great work! We have to decide on some things tho and modify couple of things in the DatePicker that were not well planned from the beginning (which is my fault).

  1. DatePicker should only consist of month input, year input and date selector. Wrapping it in a Window component with WindowHeader with hardcoded title is really limiting. That's why we should only care about making this part:

Screenshot 2020-10-18 at 12 49 41

  1. Currently we have hardcoded months and week days which will cause problems with internationalization. We should let developers pass their own labels for both months and weekday symbols.

  2. date prop should accept any parsable date (like for example string in 'YYYY-MM-DD' format), not only Date object.

  3. I have no idea how I missed it but... there's whole one row of days missing 😂 See the comparison between our component and the correctly working one (day 30 and 31 is missing):

Screenshot 2020-10-18 at 13 07 48

Screenshot 2020-10-18 at 13 08 03

This should be a minor change in code. I think we need to change 35 in this line: const dayPickerItems = Array.apply(null, { length: 35 }); to 42 since it's 6 rows * 7 days

  1. Accessibility: aria-labels and proper keyboard controls for date selection

  2. We need to have good tests in place before we release this component. I would suggest to simply copy test cases from some good datepicker library (https://github.com/Hacker0x01/react-datepicker/tree/master/test) so we don't miss anything

@luckened
Copy link

@arturbien thanks for the feedback! ❤️
I will work on it and keep you informed if I have any questions =)

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

Successfully merging a pull request may close this issue.

4 participants