Skip to content

Conversation

@arthurvi
Copy link
Contributor

@arthurvi arthurvi commented Oct 6, 2016

Instructions I was missing to get started using this date picker
See #45
closes #45

Instructions I was missing to get started using this date picker
See react-dates#45
closes react-dates#45
@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.235% when pulling 1af2423 on arthurvi:patch-1 into 8c03d77 on airbnb:master.

@vladbalan
Copy link

👍

Copy link
Collaborator

@majapw majapw left a comment

Choose a reason for hiding this comment

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

LGTM after comments are addressed! Thanks for this. This has been a long time coming. :)

README.md Outdated
```
Make sure you have the following dependencies installed:
```
npm install --save moment react react-dom react-addons-shallow-compare
Copy link
Collaborator

@majapw majapw Oct 10, 2016

Choose a reason for hiding this comment

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

While I agree this is clearer as to what is happening, the other command @ljharb added helps us specify version numbers. This command ultimately just installs the latest version of the package and not the supported version. This becomes a problem with (a) semver-major changes if we haven't audited them yet and (b) bugs in newer releases (like when moment released 1.15.0 and it broke all our tests). I would prefer to leave the slightly more confusing and more correct command here.

Copy link
Member

Choose a reason for hiding this comment

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

yes, please do not delete the command i used before.

Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to abstract this into an package.json script? some of us are on windows systems so ideally the command would be compatible with windows, as well? Also, I could not get this command to play nicely with yarn: yarn info react-dates peerDependencies --json | command sed 's/[\{\},]//g ; s/: /@/g; s/ *//g' | xargs yarn add react-dates

Here is the output for me from running just the following yarn info react-dates peerDependencies --json | command sed 's/[\{\},]//g ; s/: /@/g; s/ *//g':

"type":"inspect""data":"moment":"2.10-2.14||^2.15.1""react":">=0.14""react-dom":">=0.14""react-addons-shallow-compare":">=0.14" "type":"finished""data":630

Copy link
Contributor

Choose a reason for hiding this comment

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

oops some of my message got cut off... trace is below - it looks like type: inspectdata is throwing it off (as I imagine the final type statement will also cause errors)... not sure if this is standard output or if it's because I'm a naive Cygwin user and I'm doing something wrong - if it is standard output I'm guessing npm just ignores the type stuff?

Trace: 
  Error: type:inspectdata/:moment:2.10-2.14%7C%7C%5E2.15.1react:%3E=0.14react-dom:%3E=0.14react-addons-shallow-compare:%3E=0.14: Invalid protocol: type:
      at Request.init (C:\Program Files (x86)\Yarn\node_modules\request\request.js:460:31)
      at new Request (C:\Program Files (x86)\Yarn\node_modules\request\request.js:129:8)
      at request (C:\Program Files (x86)\Yarn\node_modules\request\index.js:55:10)
      at RequestManager.execute (C:\Program Files (x86)\Yarn\lib\util\request-manager.js:327:17)
      at RequestManager.shiftQueue (C:\Program Files (x86)\Yarn\lib\util\request-manager.js:353:10)
      at data (C:\Program Files (x86)\Yarn\lib\util\request-manager.js:251:12)
      at Request.params.callback [as _callback] (C:\Program Files (x86)\Yarn\lib\util\request-manager.js:305:11)
      at Request.self.callback (C:\Program Files (x86)\Yarn\node_modules\request\request.js:187:22)
      at emitTwo (events.js:106:13)
      at Request.emit (events.js:191:7)

Copy link
Contributor

@jpollard-cs jpollard-cs Oct 28, 2016

Choose a reason for hiding this comment

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

looking into this further - it's possible yarn has better support for installing peer dependencies via the --peer flag, but I've also put together a Powershell script for the Windows users - should this be the expected output?

npm install --save react-dates moment@"2.10-2.14||^2.15.1" react@">=0.14" react-addons-shallow-compare@">=0.14" react-dom@">=0.14"

here is the script to produce that - it's not as clean and elegant as your unix script, but it gets the job done:

$ErrorActionPreference = "SilentlyContinue"
$json = (npm info react-dates peerDependencies --json) | ConvertFrom-Json
$props = $json.data | Get-Member -membertype properties
$command = "npm install --save react-dates ";
foreach($prop in $props)
{
    $command += $prop.Name + "@`"" + (($json.data | Select -ExpandProperty $prop.Name) -replace '\s') + "`" "
}
Invoke-Expression $command

(note I'm not a frequent powershell user so there may be room for improvement here... there's also probably a more canonically correct way of doing this via npm packages that would work across operating systems)

Copy link
Contributor

Choose a reason for hiding this comment

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

@majapw @ljharb thoughts on the above? seems like arthurvi isn't going to update this PR so I can pick up where he left off

Copy link
Member

Choose a reason for hiding this comment

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

Let's keep the install script as it is in the readme.

If you'd like to put up a replacement PR, that'd be great!

Copy link
Contributor

Choose a reason for hiding this comment

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

@ljharb alright I'm good with this now that I see it runs in Git Bash... I couldn't get it to run under Cygwin or Cmder which is why I put the script together in the first place.. will put in an updated PR soon

Copy link
Contributor

Choose a reason for hiding this comment

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

@ljharb I'm mostly concerned with clarifying the install which seems to be accounted for here: #168 so does not like I'll need to submit a PR anymore ... the rest of this stuff should be clarified by examples and is pretty standard

README.md Outdated
#### Make some awesome datepickers
```
<SingleDatePicker
id="date_input"
Copy link
Collaborator

@majapw majapw Oct 10, 2016

Choose a reason for hiding this comment

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

nit: the Airbnb JS style guide prefers 2-space indentation. :P

@vladinator1000
Copy link

This is super helpful!

@arthurvi
Copy link
Contributor Author

arthurvi commented Nov 25, 2016

Whoops! Totally forgot about this one!
I think it is done, should be merged with a squash I guess.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.04% when pulling 30baa9e on arthurvi:patch-1 into 2d86825 on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.04% when pulling 30baa9e on arthurvi:patch-1 into 2d86825 on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.04% when pulling 30baa9e on arthurvi:patch-1 into 2d86825 on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 87.04% when pulling 30baa9e on arthurvi:patch-1 into 2d86825 on airbnb:master.

@majapw
Copy link
Collaborator

majapw commented Nov 29, 2016

Hey @arthurvi can you squash it down yourself and push it back up? I'll merge it in after that. :)

@arthurvi arthurvi closed this Dec 1, 2016
@arthurvi arthurvi deleted the patch-1 branch December 1, 2016 08:43
@arthurvi arthurvi restored the patch-1 branch December 1, 2016 08:44
@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.657% when pulling 1557fa1 on arthurvi:patch-1 into 32bb58d on airbnb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 86.657% when pulling 1557fa1 on arthurvi:patch-1 into 32bb58d on airbnb:master.

@arthurvi
Copy link
Contributor Author

arthurvi commented Dec 1, 2016

New PR: #202
@majapw It can be merged now I guess! I got a little over enthusiastic in squashing things in the previous branch, so here is a cleaner version... 😬

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

Successfully merging this pull request may close these issues.

Getting started instructions

7 participants