Navigation Menu

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

Revamped the README file #21744

Closed
wants to merge 8 commits into from
Closed

Conversation

javiereguiluz
Copy link
Member

Q A
Branch? 2.7
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #21235
License MIT
Doc PR

Here is a before/after comparison image:

before-after-readme

@fbourigault
Copy link
Contributor

A reference to the symfony-demo project seems relevant :) If the demo is hosted live somewhere, linking to this live hosted version would be awesome!

@Wirone
Copy link
Contributor

Wirone commented Feb 24, 2017

What about adding some badges about code coverage or build status?

@stof
Copy link
Member

stof commented Feb 24, 2017

@Wirone Fabien has always been against badges in the readme.

@javiereguiluz there is nothing talking about community reviews anymore. Is it intended ?

@Wirone
Copy link
Contributor

Wirone commented Feb 24, 2017

@stof ok, I didn't know. Just asking ;-)

@stof
Copy link
Member

stof commented Feb 24, 2017

Due to composer/packagist#764, the rendering of the readme on Packagist will become weird with this new content, as it will remove the Installation here.
We should either keep a top-level title, or contribute a fix in Packagist.

README.md Outdated
README
======
<p align="center"><a href="https://symfony.com" target="_blank">
<img src="http://symfony.com/logos/symfony_black_02.svg">
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding logo to resources, for example in FrameworkBundle and don't rely on external content? There is Symfony svg in WebProfilerBundle but not black and smaller.

Copy link
Member

@stof stof Feb 24, 2017

Choose a reason for hiding this comment

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

the SVG icons in WebProfilerBundle are part of the features being delivered (as they are displayed in the UI).
However, adding an image in the repo just for the readme looks weird to me.

Copy link
Member Author

Choose a reason for hiding this comment

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

We could do that ... but we prefer to link to the symfony.com logo because it gives us more control and flexibility (we could change it for some special occasion, etc.)

Copy link
Contributor

Choose a reason for hiding this comment

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

@stof maybe weird, but stable ;-)

@javiereguiluz Ok, I understand, but maybe it should be dedicated resource (symfony_repo_header.svg instead of symfony_black_02.svg)? As far as everyone knows the symfony_black_02.svg is displayed here it's ok, but file could be renamed or deleted.

It allows you to start a new project based on the version you want.
* [Install Symfony][4] with Composer or with our own installer (see
[requirements details][3]).
* Symfony follows the [semantic versioning][5] strictly, publishes "Long Term
Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I don't think it's about installation, rather about way of developing. I think it should be in different section.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. This one is tricky. It's not about installation ... but it's about how are Symfony versions released ... which is what you really install/update/use. Creating a new section looked like too much for this.

Copy link
Contributor

Choose a reason for hiding this comment

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

So maybe rename section to Installation & maintanance?

README.md Outdated
[8]: https://symfony.com/doc/current/contributing/community/reviews.html
Symfony is an Open Source, community-driven project. Join us [contributing code][17]
or [contributing documentation][18] and you'll end up in the [Symfony contributors][19]
Hall of Fame.
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't find this funny, but it's subjective ;-)

Copy link
Member Author

Choose a reason for hiding this comment

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

The intention was not to be funny 😐 "Hall of Fame" is a commonly used name to refer to the place where you recognize the merits of some special people. See https://en.wikipedia.org/wiki/List_of_halls_and_walks_of_fame

Copy link
Contributor

Choose a reason for hiding this comment

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

I know what is "Hall of Fame" (hey, NBA), but I find this phrase trying to look cool and unofficial. But as I said, it's really subjective.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think you are right. In ec8cc27 I've reworded this section. Thanks.

README.md Outdated
README
======
<p align="center"><a href="https://symfony.com" target="_blank">
<img src="http://symfony.com/logos/symfony_black_02.svg">
Copy link
Member

Choose a reason for hiding this comment

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

you should use a HTTPS URL here, even though it is not an issue on github as they proxy all images since a long time to avoid mixed content (and other risks)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch! Other than that, the only link that doesn't use HTTPS is semver.org ... because they seem to have some issues.

Copy link
Member

Choose a reason for hiding this comment

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

external links are less an issue regarding mixed content 😄

Copy link
Member

Choose a reason for hiding this comment

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

regarding HTTPS on semver.org, this is because they are hosted on github pages, and github pages don't support HTTPS for custom domains (the certificate is valid only for the github.io domains)

@javiereguiluz
Copy link
Member Author

Answering the pending comments:

@fbourigault I've added a mention to the Symfony Demo (in the docs section ... I can't find a better place)
@stof yes, let's remove the community reviews because it's a second-level information. We link to contribute code/docs, which is the important thing, and there we link to the community reviews, etc.
@Wirone we know that badges are popular, but for Symfony we decided that they don't provide much useful information.
@stof yes, let's fix the Packagist issue in their repository


So, we're good to go :)

@fabpot
Copy link
Member

fabpot commented Feb 24, 2017

Thank you @javiereguiluz.

fabpot added a commit that referenced this pull request Feb 24, 2017
This PR was squashed before being merged into the 2.7 branch (closes #21744).

Discussion
----------

Revamped the README file

| Q             | A
| ------------- | ---
| Branch?       | 2.7
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #21235
| License       | MIT
| Doc PR        |

Here is a before/after comparison image:

![before-after-readme](https://cloud.githubusercontent.com/assets/73419/23294444/cb001e9a-fa6b-11e6-88f2-a8449470fb4e.png)

Commits
-------

c7d30ca Revamped the README file
@fabpot fabpot closed this Feb 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants