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

[book] 1st to 4th ch changes #6252

Closed
wants to merge 6 commits into from
Closed

[book] 1st to 4th ch changes #6252

wants to merge 6 commits into from

Conversation

talitakz
Copy link
Contributor

Q A
Doc fix? yes
New docs? no
Applies to all
Fixed tickets non

formating
links
references
better explanations
unified some repeted text
@@ -248,7 +256,7 @@ and reusable. To prove it, add a blog "show" page, which displays an individual
blog post identified by an ``id`` query parameter.

To begin, create a new function in the ``model.php`` file that retrieves
an individual blog result based on a given id::
an individual blog result based on a given ``id``::
Copy link
Member

Choose a reason for hiding this comment

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

I don't think this needs to be a code block here actually

you. Instead of *telling* you that Symfony allows you to develop faster and
better software than with flat PHP, you'll see for yourself.
If you've never used a PHP framework, aren't familiar with the
model-view-controller `MVC`_ philosophy, or just wonder what all the *hype*
Copy link
Member

Choose a reason for hiding this comment

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

I would change this to: "Model-View-Controller (MVC) philosophy" also, we don't use a serial comma (the one before "or") in the docs. Although that isn't your fault, it would be nice to include it in this PR

@talitakz
Copy link
Contributor Author

I made requested changes.
If there are no other remarks this could probably be merged.

@@ -98,11 +102,12 @@ the code that prepares the HTML "presentation"::
require 'templates/list.php';


The HTML code is now stored in a separate file (``templates/list.php``), which
The HTML code is now stored in a separate file ``templates/list.php``, which
Copy link
Member

Choose a reason for hiding this comment

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

Changing the order to "in a separate templates/list.php file" seems to be better (although I'm not a native).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree.

@talitakz talitakz changed the title [] changes in the first four ch of the book [book] 1st to 4th ch changes Mar 12, 2016
@talitakz talitakz changed the title [book] 1st to 4th ch changes [book] 1st to 5th ch changes Mar 12, 2016
@talitakz talitakz changed the title [book] 1st to 5th ch changes [book] 1st to 4th ch changes Mar 12, 2016
@talitakz
Copy link
Contributor Author

I would appreciate a final response so this could be finally merged. Thank you!

@weaverryan weaverryan mentioned this pull request Jul 10, 2016
weaverryan added a commit that referenced this pull request Jul 10, 2016
This PR was merged into the 2.8 branch.

Discussion
----------

Finishing #6252

WIP - finishing #6252

Re-opening so I can tweak and review. Rebased to 2.8

Commits
-------

68da559 Removing old :term: stuff
f67f675 Replacing missing link
bafea9d Fixing typo
7e9dc86 Final changes
a527a43 [WIP] More changes
c390d8d [WIP] More changes
6ae19f0 [WIP] Removing multiple formats - it makes no sense here
1297c75 [WIP] More changes
47ffb4e [WIP] Reading through the changes
09b28bc changes according to comments after PR + some other typos
689c2fd typo fix in hyperlink
49ec40f typo fix in hyperlink
62f5153 colon  corrections
c56db5c made some new changes, corrected mistakes, undo some foolish deletions
2dc8c63 [] changes in the first four ch of the book
@weaverryan
Copy link
Member

Hey @paxyknox!

Ok, I've just merged this in! Actually, I created a new PR - #6743 - so I could rebase, make changes their and see the diff.

The reviews you did and PR's you made are great... but also really tough for us (hence the long delay). Merging just this PR took me almost 2 hours. The problem is their size: your PR's are huge, and contain a lot of good changes, but also a lot of changes that I need to remove (e.g. the :: that you added in many places is actually incorrect).

So unfortunately, I will be closing all of your PR's - we simply can't review them in a timely fashion. This is a shame because there's good stuff in there.

In the future, to help us out, here's what we need:

A) Smaller PR's, both in general size and in how many changes show up. For example, if you change the indentation on a section (or change something from a sidebar to not be a sidebar), that change looks huge in the PR. If you only made a PR with this change and said "I just changed this to not be a sidebar, but didn't update the text" - then that's an easy change :).

B) Organize PR's by what your modifying. For example, if you're rewording some things to improve their wording, only do that - don't also fix other things. Or, if you're adding more links or :class links to be helpful, only add those. When things are mixed, simple, nice improvements get blocked while we discuss and try to modify the bigger stuff. Or, conversely, a nice re-wording might be blocked by other incorrect changes (like adding the :: in places where it shouldn't be).

Cheers!

@weaverryan weaverryan closed this Jul 10, 2016
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.

5 participants