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

update the plain-javascript README.md #458

Closed
wants to merge 5 commits into from
Closed

update the plain-javascript README.md #458

wants to merge 5 commits into from

Conversation

johnnyreilly
Copy link
Contributor

Hi!

I'm just investigating the migration story from 0.x to 1.x and as a first step I thought I'd give the plain JavaScript example a go. I got there after a little fiddling and so I thought I'd try and clarify the steps required in the README.md. I hope that's okay?

Best,
John

@johnnyreilly
Copy link
Contributor Author

Hi @rxaviers / @jzaefferer,

I've tried to submit a docs change (just a README.md change) and have already fallen foul of the Continuous Integration build. The following slightly cryptic messages have been thrown up:

  • First line (subject) must be no longer than 72 characters
  • First line (subject) must indicate the component

Could you tell me what these errors refer to please? I'd be happy to fix them up if I knew!

@jzaefferer
Copy link
Contributor

That's about the commit message, you can just ignore that. Or do a git commit --amend to try to address it.

@johnnyreilly
Copy link
Contributor Author

Thanks @jzaefferer - I'll ignore it as advised!

└── index.html
└── README.md
Copy link
Member

Choose a reason for hiding this comment

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

Can you update └── index.html for ├── index.html please? The same with supplemental.js.

@rxaviers
Copy link
Member

@johnnyreilly thank you very much for your clarifying improvements. I have left a couple of comments above. Just let me know on any questions.

Thanks

@rxaviers
Copy link
Member

@johnnyreilly just wondering if you had a chance to look at my comments? Thanks

@johnnyreilly
Copy link
Contributor Author

Hi @rxaviers,

I've made changes following your comments. I've followed all your suggestions to the letter apart from the last. I think the 2 separate examples are valuable as they are - I hope that's okay? I've slightly rejigged it for clarity though.

@jzaefferer
Copy link
Contributor

@johnnyreilly for some reason your last commit uses a different author name ("johnnyreilly" instead of "John Reilly"), that's why it no longer passes our CLA check. You could update that with `git --amend --author=... or squash all commits into one, as long as the remaining commit has the right author information. Or I guess @rxaviers could do that for you, when landing this.

@johnnyreilly
Copy link
Contributor Author

Tried and failed @jzaefferer 😢 Hopefully @rxaviers can sort it for me?

@rxaviers
Copy link
Member

rxaviers commented Aug 4, 2015

Hi @johnnyreilly, I can squash your commits and keep "John Reilly" as your correct one.

@johnnyreilly
Copy link
Contributor Author

Great - go for it!

@@ -22,7 +22,7 @@ you want. But, as an exercise of this demo, we'll download it ourselves. So:
1. Click at [cldrjs releases tab](https://github.com/rxaviers/cldrjs/releases).
1. Download the latest package.
1. Unzip it.
1. Move its `dist/` files into `cldrjs` of this directory.
1. Create a `cldrjs` directory alongside `index.html` and `README.md` and move the cldrjs `dist/` files into it.
Copy link
Member

Choose a reason for hiding this comment

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

@johnnyreilly, I believe we can improve this sentence for clarity.

@kswedberg do you have a suggestion to help us out here?

Choose a reason for hiding this comment

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

Looks fine to me, @rxaviers . One thing that might make it more clear would be to break it up into 2 steps:

...

  1. Inside the unzipped directory, rename the dist directory to cldrjs.
  2. Move the renamed cldrjs directory from the unzipped folder to this demo's directory so that it appears alongside index.html and README.md.

I don't know, though. Maybe that just makes it overly complicated.

Copy link
Member

Choose a reason for hiding this comment

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

What I found confusing was having cldrjs to reference to two different things: the empty just-created directory and the unzipped directory. But, we can ignore my comment considering you both think this is clear. :)

Thanks @kswedberg for your prompt answer.

@rxaviers
Copy link
Member

rxaviers commented Aug 7, 2015

I've made changes following your comments. I've followed all your suggestions to the letter apart from the last. I think the 2 separate examples are valuable as they are - I hope that's okay? I've slightly rejigged it for clarity though.

Looks good to me.

I have left two comments though in other sentences. Sorry for the ping pong, which delays landing your PR. But, hopefully my last observation/suggestion.

│ └── cldr
│ ├── event.js
│ └── supplemental.js
└── index.html
│ └── unresolved.js
Copy link
Member

Choose a reason for hiding this comment

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

unresolved.js isn't used by this example, therefore I think it shouldn't be listed here (for simplicity).

@rxaviers
Copy link
Member

Thank you @johnnyreilly

rxaviers pushed a commit that referenced this pull request Aug 10, 2015
@rxaviers rxaviers closed this in 25d064c Aug 10, 2015
rxaviers added a commit that referenced this pull request Aug 10, 2015
rxaviers added a commit that referenced this pull request Aug 10, 2015
rxaviers pushed a commit that referenced this pull request Jan 17, 2016
rxaviers pushed a commit that referenced this pull request Jan 17, 2016
rxaviers added a commit that referenced this pull request Jan 17, 2016
rxaviers added a commit that referenced this pull request Jan 17, 2016
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.

5 participants