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

Adding maven badge to readme #381

Merged
merged 9 commits into from
Jan 14, 2018
Merged

Adding maven badge to readme #381

merged 9 commits into from
Jan 14, 2018

Conversation

fleipold
Copy link
Contributor

This PR converts the README file to markdown and adds a maven badge, so that the latest version is immediately visible.

Somewhat like this:
image

It also adds a little bit of markdown syntax maintain a reasonable appearance (.md-files get rendered in non-fixed font by default).

@johnjaylward
Copy link
Contributor

@stleary, it may finally make sense to have the markdown version. Having those "badges" at the top can be handy.

@stleary
Copy link
Owner

stleary commented Nov 14, 2017

@johnjaylward Agreed, a markdown readme would be a helpful improvement.
@fleipold please change the class name highlighting from inline-code to bold. The badge is good, but please move it to the bottom of page.
@anyone - additional comments or suggestions are welcome.

@fleipold
Copy link
Contributor Author

@stleary Thanks for the quick feedback. I am happy to move the badge around and to make the class names bold. However there seems to be a convention to have the badges at / near the top. What do you think?

I will not get to this until the end of the week anyway, so I'll check for more feedback and could then incorporate that too.

@stleary
Copy link
Owner

stleary commented Nov 14, 2017

@fleipold yes badge at the top is OK.

@fleipold
Copy link
Contributor Author

fleipold commented Nov 17, 2017

@stleary I have made the file names (not the class names). Is that ok for you? I also made a couple of links markdown links. You can preview the rendering on my branch. Let me know what you think.

@stleary
Copy link
Owner

stleary commented Jan 10, 2018

@fliepold Apologies, I got out of sync on this. Please submit the pull request if you still want to commit this change. The updates you have described should be fine.

@fleipold
Copy link
Contributor Author

@stleary No worries. To me it looks like this pull request is still up-to-date, so you should be able to merge it by clicking the merge button (you can also squash all the commits into one, I just made a lot of commits for clarity, but perhaps they shouldn't go to the master). The options how to merge a pull request are described here: https://help.github.com/articles/merging-a-pull-request/#merging-a-pull-request-on-github

@stleary
Copy link
Owner

stleary commented Jan 11, 2018

Thanks for the refresher :) PR will be merged soon, pending comment window.

@stleary
Copy link
Owner

stleary commented Jan 12, 2018

What problem does this code solve?
Not a bug fix or enhancement, just improving the Readme.

Risks
N/A

Changes to the API?
N/A

Will this require a new release?
N/A

Should the documentation be updated?
This is part of the documentation

Does it break the unit tests?
N/A

Was any code refactored in this commit?
N/A

Review status
APPROVED Starting 3 day window for comments

@stleary stleary merged commit ca45b02 into stleary:master Jan 14, 2018
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.

3 participants