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

feat(datetime): Add component #1240

Closed
wants to merge 30 commits into from
Closed

Conversation

harel
Copy link

@harel harel commented Jan 27, 2017

This is a work in progress mainly focused on exploring the possiblity of adding a datetime component which supports date ranges, time and localisation.

TODO

Bugs and Features

  • allow typing/pasting dates into the input field
  • do not show completely disabled weeks in month view
  • Menu needs some work, buttons don't make sense in all states
  • Expose ways to customize the menu and table as well
  • Add some more props for configuring the view and doc examples
    • showWeekends
    • showDaysOfWeek
  • if at all possible, remove inline styles. required styles should be configurable.
  • move to 5 button CalendarMenu: << < Mo, Yr > >>. double chevrons change year, single change month, mo
  • don't allow selecting invalid ranges: start > end
  • use 12hr time grid with AM/PM option
  • provide onChange callbacks with (e, { value }) where value is the proposed new value.
  • Add dateSeparator prop to DateRange to control the separator string between the two dates

Docs

  • document function prop signatures
  • change Week starts Sunday example to First Day Of Week show two examples, start on Sun, another on Monday.
  • fix disabled days example, do not import date utils

Cleanup

  • move component to /addons
  • remove cruft propTypes that do not exist
  • sort propTypes alphabetically
  • update Month.js to use DateTimeGrid, feat(datetime): Add component #1240 (comment)
  • complete all code TODOs in /Datetime
  • reverse onDateSelect args to match other callbacks, with event first.
  • remove unused methods in classes

Copy link
Member

@levithomason levithomason left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I realize this is work in progress. Couple typos that might trip you up:

return <ElementType {...rest} className={classes}>{children}</ElementType>
}

DropdownMenu._meta = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CalendarMonth._meta

type: META.TYPES.MODULE,
}

DropdownMenu.propTypes = {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CalendarMonth.propTypes

@harel
Copy link
Author

harel commented Jan 30, 2017

That component wasn't imported yet but fixed the typos. Did not solve the problem.

image

@levithomason
Copy link
Member

Thanks for the note, I'll take a look at this branch later tonight and resolve the issue.

@codecov-io
Copy link

codecov-io commented Jan 30, 2017

Codecov Report

Merging #1240 into master will decrease coverage by 80.65%.
The diff coverage is 39.78%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #1240       +/-   ##
===========================================
- Coverage   99.73%   19.07%   -80.66%     
===========================================
  Files         151      159        +8     
  Lines        2624     2862      +238     
===========================================
- Hits         2617      546     -2071     
- Misses          7     2316     +2309
Impacted Files Coverage Δ
src/modules/Dropdown/Dropdown.js 0.51% <ø> (-99.49%) ⬇️
src/addons/Datetime/DatetimeRange.js 1.38% <1.38%> (ø)
src/addons/Datetime/Datetime.js 23.33% <23.33%> (ø)
src/addons/Datetime/DatetimeMenu.js 28.57% <28.57%> (ø)
src/addons/Datetime/DatetimeHours.js 30.76% <30.76%> (ø)
src/addons/Datetime/DatetimeCalendar.js 31.81% <31.81%> (ø)
src/addons/Datetime/DatetimeMonths.js 62.5% <62.5%> (ø)
src/addons/Datetime/DatetimeGrid.js 64.28% <64.28%> (ø)
src/addons/Datetime/DatetimeYears.js 76.92% <76.92%> (ø)
src/addons/Datetime/DatetimeMinutes.js 81.25% <81.25%> (ø)
... and 162 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 470c7b5...f1a8e0d. Read the comment docs.

@levithomason
Copy link
Member

levithomason commented Jan 30, 2017

Fixed:

-static PropTypes
+static propTypes

I also pushed a lot of cleanup so I could browse the code a little easier.

@levithomason
Copy link
Member

Looks like we'll need to reabse this branch to master to get the commits in order. All the typings were moved from index.d.ts files to individual component files such as Button.d.ts. There is still an index.d.ts file but it just exports the default component file from the directory.

This is 1:1 with how the JS files are setup as well. LMK if you run into any issues with the rebase cleanup and I'll help out.

@harel
Copy link
Author

harel commented Mar 6, 2017

No worries. I rebase periodically anyway so this will be just one more link in the chain. Didn't do much over the last few days because of life but i'm back at it this week.

@layershifter layershifter changed the title datetime(changelog): initial commit for datetime component experiments feat(datetime): Add component Mar 21, 2017
@levithomason levithomason force-pushed the datetime branch 2 times, most recently from 4d54f2f to 87d50ed Compare March 26, 2017 05:12
@levithomason
Copy link
Member

I've pushed some cleanup and a rebase to the latest master.

I think we should start breaking this PR down into small shipments and releasing it. This will allow us to focus on one chunk at a time and do a good job on just that. As we polish and release each piece, we can bring it back into this branch and prepare the next release.

Do you think we could somehow release Date only as the first component, the Time, and Datetime? I'm wondering if it makes sense to break these down that way API wise as it does release wise. Let me know your thoughts.

@levithomason
Copy link
Member

Heads up, I'll be pushing some more refactors and cleanups for a bit here.

@levithomason
Copy link
Member

OK, after pushing the latest I think we can ship Datetime with basic support first, then perhaps follow-up with the range support.

DateTimeGrid

I pushed a generic DateTimeGrid that is used in all but the Month view. It allows an easier construct the table cells, just pass a single array of as many cells as you want and tell the DateTimeGrid how many columns to fit them into. It will chunk out the Rows for you.

This will also give us the ability to handle different layouts, since the DateTimeGrid just takes a single array of cell props objects. We can use this array to build any layout. We can even, later on, expose a renderGrid prop and allow the user to render their own calendar grids if we wanted to.

TODO

I'd like to factor out the DayCell into the DateTimeGrid but am not 100% sure on how yet. I'd also like to move the dateUtils to the Datetime directory and perhaps remove more, if possible (I removed some already).


That's all I got for now, thanks for all the work on this and the patience dealing with my limited availability. We're getting close to something here 👍

@KevinGorjan
Copy link

Whats the status of this issue?

@GregoryPotdevin
Copy link

I'm also interested in this feature and can dedicate some time this week to test/code/document/brainstorm or whatever is needed.

@harel
Copy link
Author

harel commented Apr 25, 2017 via email

@viiiprock
Copy link

Hi guys, how is it going :)

@revik
Copy link

revik commented Nov 6, 2017

Can't wait, any progress ? Do you need help testing it?

@dankirlin
Copy link

Let me know if there is anyway I can help push this forward.

@ydsood
Copy link

ydsood commented Nov 6, 2017

Waiting eagerly for this to be completed. This is a very frequently used component in forms. Available for helping push this out soon.

@neslinesli93
Copy link
Contributor

Same here, count me in if you need anything

@levithomason
Copy link
Member

If anyone would like to hop in here's some more info. Conformance tests are passing and there are plenty of doc site examples. What's needed is to run the doc site, try the examples, and fix the bugs. Bug fixes should also have tests written for them.

As soon as all the doc site examples are working with passing tests, this can be shipped. 🎉

@levithomason levithomason mentioned this pull request Dec 12, 2017
@srdjanRakic
Copy link

How is it going guys ? Any news on this ?

@Gallevy
Copy link

Gallevy commented Dec 20, 2017

Patiently waiting. Anything I can do to speed this up?

@davidhenley
Copy link

This is the final piece missing for all my forms using SUI. Can't wait!

@hisuwh
Copy link

hisuwh commented Dec 22, 2017

All I want for Christmas is SUI datepicker 🎄 🎁 📆 😆

@brianespinosa
Copy link
Member

Hey everyone! I think we are all excited to ship this. At the same time, there is only so much bandwidth that main contributors to this project have to give to open source work. The nice thing is that it's open source and we are happy to accept pull requests from anyone that moves this project/feature forward.

Per @levithomason's comment above, as soon as doc site examples are working and tests are passing for the reported bugs, this can be shipped. @Gallevy you asked if there is anything you can do to move this forward... we welcome any time you can dedicate to fixing any of the remaining bugs. Anyone else who has commented wanting to know what is going on, you are also welcome to help out. <3

@srdjanRakic
Copy link

@brianespinosa When will we be able to use this? :)

@therealgilles
Copy link

Could this PR be merged so that testing it is made easier? Thank you

@levithomason
Copy link
Member

If you'd like to contribute to the development of this feature, please email me[at]levithomason.com. This thread produces too much noise and distracts from the focus.

@Semantic-Org Semantic-Org locked as spam and limited conversation to collaborators Feb 4, 2018
@levithomason
Copy link
Member

A few collaborators have emailed me and pushed this work forward. You are amazing ❤️.

I've ported this branch to our repo for easier management. I'm merging any well authored PRs against this effort.

See #2821 for the Datetime branch on the main repo
See #2813 as an example of how you can help in making PRs against the feature branch

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

Successfully merging this pull request may close these issues.