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

Launching V1.0 #749

Closed
1 of 3 tasks
martijnrusschen opened this issue Feb 25, 2017 · 25 comments
Closed
1 of 3 tasks

Launching V1.0 #749

martijnrusschen opened this issue Feb 25, 2017 · 25 comments

Comments

@martijnrusschen
Copy link
Member

martijnrusschen commented Feb 25, 2017

About a year ago, we did our latest try on launching a v1.0 version. I think it would be good to define a v1.0 version so that we can apply breaking changes when needed and show the maturity of this component.

Our last effort in defining what should be done before we launch v1.0 was documented in https://github.com/Hacker0x01/react-datepicker/milestone/2. What do you guys think of this? Should be continue with this, create a new Milestone, or just call it a day and launch v1.0 in the current state?

Todos:

  • Webpack2?
  • Remove grunt?
  • Decide if we leave the API as it is?
@martijnrusschen
Copy link
Member Author

Adding some recent contributors for their opinion on this: @mgrdevport, @jochenberger, @ro-savage, @rafeememon.

@jochenberger
Copy link
Contributor

I haven't used react-datepicker in a while because I had too many issues with it, so I don't have a strong opinion.
You might want to consider doing some alpha/beta releases before 1.0.

@ro-savage
Copy link
Contributor

ro-savage commented Mar 1, 2017

@martijnrusschen
Unfortunately the portal doesn't work as expected on iphone iOS 10.

When you click the datepicker, the portal should be fixed, take up the entire screen, and remove the ability to scroll.

This works on all devices we tested including iPad iOS10 but does not work on iPhone iOS10, instead the portal appears at the top of the page, and the viewport stays focused on the the input.

My guess is this is something to do with focus being on the input at the same time it tries to change the viewport, and by not focusing on the input when clicked (or focusing on the calendar) it may fix it.

We worked around it by using the built-in date picker when we detected an iPhone.

Unfortunately, I am not at the company anymore and no longer have access to an iPhone to test.

Maybe release with a known issue, and offer the work around until its fixed?

@rafeememon
Copy link
Contributor

IMO the most important thing to settle on is the API, which includes the components we will expose, the props that they'll take in, and possibly even the styles. I think it stands to be cleaned up pretty substantially, and there are a few breaks that we've wanted to make but deferred on. Once we've settled on the API I think we'll have a clear path forward to getting to a releasable v1.

@ro-savage
Copy link
Contributor

ro-savage commented Mar 2, 2017

General feedback:

  • We used quiet a bit of the API and found it to be good and completed. I wouldn't see any major breaking changes from where it is now.
  • withPortal sounds like a stupid name to me, but I used the same name as react-dates
  • I may be wrong, but I think you have to pass the date picker a moment object. Would be nice to pass a regular JS Date object and it get's handled.
  • Moment is great but also huge file size. In our case, we didn't need internationalise or timezones. So it was a big thing to include. Don't really have an alternative. (We used webpack to exclude what we didn't need)
  • The library itself is large coming in at 80.7kb. It is our largest outside react-dom.
    • It looks lots of lodash is being included which is contributing 20.1kb, tether.js is 24.9kb. The source itself is 29.6kb (which still feels rather large).
    • There is probably ways to decrease the overall sizes quiet a bit.
  • Would be great to not have the extra days of a month on the calendar. Either they are missing or greyed out.
  • CSSModules is working great, this was the first project that I converted the css to cssmodules automatically as a seperate file, and I have since done it again and will continue to (its not the cleanest thing, but CSSModules is still trying to figure out best way to get css + cssmodules to play nice by default)

Source map explorer of react-datepicker
image

@ro-savage
Copy link
Contributor

It should be pretty easy to remove lodash completely.
Just need to add a polyfill for find, about 300 bytes. And everything else can be replaced with native.

Let me know if you want a PR for removing lodash.

@rafeememon
Copy link
Contributor

rafeememon commented Mar 3, 2017

@ro-savage, if find is the last thing let's just implement it with filter; it doesn't seem like good practice for a library to install a polyfill and it's one less dependency to worry about

@ro-savage
Copy link
Contributor

Done :)

@martijnrusschen
Copy link
Member Author

IMO the most important thing to settle on is the API, which includes the components we will expose, the props that they'll take in, and possibly even the styles. I think it stands to be cleaned up pretty substantially, and there are a few breaks that we've wanted to make but deferred on. Once we've settled on the API I think we'll have a clear path forward to getting to a releasable v1.

We might just leave it as it is, if there's no urgent reason to do it. I agree that API is the most important piece to settle on, but looking at the comments it seems pretty decent currently.

@aij
Copy link
Contributor

aij commented Mar 17, 2017

@martijnrusschen I think the recentish API changes wrt timezones has created a fair bit of confusion. (Eg: #747 #727 #627) It doesn't help that the API is undocumented in regards to what sort of moment objects are expected, and a lot of existing examples violate those undocumented expectations. (For example, by passing in local moment objects, or by assuming the moment object received from onChange is suitable to pass back in via selected.)

@alex-shamshurin
Copy link
Contributor

Also we need new build system. Webpack2 and react-theme support.

@martijnrusschen
Copy link
Member Author

I'd love to get rid of grunt and move everything to webpack. However, I don't have the time to convert the scripts. I have a WIP open to migrate to webpack 2: #735.

@martijnrusschen
Copy link
Member Author

@aij What would you do to make that easier to understand? I'm open for suggestions.

@alex-shamshurin
Copy link
Contributor

Oh, and along with webpack2 I meant to change the building itself. Of course without grunt and I do not understand why there after each build I get some bundle.js (I think it's a demo site) file and this is included into git.

@martijnrusschen
Copy link
Member Author

@alex-shamshurin Correct, the demo site generates a bundle.js which is committed to the gh-pages branch. I think we should just add it to the ignore list for the master branch. Thoughts?

@alex-shamshurin
Copy link
Contributor

Sure, I think demo site must be updated only when you release new version. May be it's the only case of grunt..

@martijnrusschen
Copy link
Member Author

It helps to have a demo site to use as a local development environment. You can directly see and test your changes.

@alex-shamshurin
Copy link
Contributor

Anyway it may must excluded from git.

@alex-shamshurin
Copy link
Contributor

And another issue with I had problems it's an internal state, It works async and sometimes it leads to strange behaviour. It's better to use exterrnal state (passed as a prop) or use internal mobx to make it sync.

@aij
Copy link
Contributor

aij commented Mar 27, 2017

@martijnrusschen I think the main thing that needs to happen with regards to how the API uses moment, is that someone needs to make a decision. That decision should be documented somewhere, and the code and/or examples should be updated to match.

I ended up opening #781 where I tried to explain the confusion in detail. TL;DR: The current code seems to assume all dates are passed in as local moment objects, yet onChange will send back a mix of local and fixed-UTC-offset moments, and hence the examples violate that assumption. No one seems to know what is correct, because no one has made and documented such a decision, so the interpretation of the moment props keeps getting completely changed (aka "fixed") with little to no notice.

@jzaefferer
Copy link

Having used react-datepicker for a few months: I suggest making the next release 1.0, without any further changes. Then follow usual semver from there. Its going to be fine.

@alex-shamshurin
Copy link
Contributor

I think it's not stable enough. We have issues with build system, internal moment (see the post above), sync state and some other errors

@ro-savage
Copy link
Contributor

Its been a while since I used react-datepicker but we struggled with the way datepicker uses moment as well.

From memory copied our moment object into a new moment object, passed it to datepicker, then copied the result back into another moment object. This was done to ensure correct timezones. (Or something).

Whatever it was, we struggled for a long time to get our dates correct with react-datepicker given that we are in New Zealand. It would be great if timezones were handled better. (Offset isn't enough, due to daylight savings)

@jzaefferer
Copy link

Newest release: 0.64.0. Oh well. Good to see the project is still moving forward though 👍

@martijnrusschen
Copy link
Member Author

Getting closer. V1 should be ready before the end of the year.

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

No branches or pull requests

7 participants