-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
[DomCrawler] Document images crawler #4971
Conversation
valeriangalliat
commented
Feb 7, 2015
Q | A |
---|---|
Doc fix? | no |
New docs? | yes (symfony/symfony#13649, symfony/symfony#13650) |
Applies to | 3.0.0 |
Fixed tickets | symfony/symfony#12429 |
@valeriangalliat Thank you for creating the doc PR. FYI, I have prefixed the PR title with "WCM" (waiting code merge). |
~~~~~~ | ||
|
||
To find an image by its ``alt`` attribute, use the ``selectImage`` method on an | ||
existing crawler. This returns a Crawler instance with just the selected |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please enclose "Crawler" with double backticks too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it even be a :class:
Symfony\Component\DomCrawler\Crawler``?
Also the "Links" paragraph has the same "Crawler" without backticks nor :class:
, should I make another PR to fix it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm -1 for this, as literals always break the read flow, making text harder to read. We usually only add an API doc link once in the article.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We usually enclose class names with literas. Why do you not want to do this here?
@valeriangalliat Would you mind updating the description with references to the new pull requests? |
@xabbuh Done! |
Thank you! |
…riangalliat) This PR was squashed before being merged into the 3.1-dev branch (closes #17585). Discussion ---------- [DomCrawler] Abstract URI logic and crawl images | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #12429 | License | MIT | Doc PR | symfony/symfony-docs#4971 This is a backward-compatible version of #13620, and a rebase of #13649 on current `master`. Commits ------- 1553b07 [DomCrawler] Abstract URI logic and crawl images
Except for the minor pending comment from @xabbuh, I'm 👍 for this change. By the way, this is no longer |
@javiereguiluz Thanks, I updated the PR according to @xabbuh comment. |
Thanks for not only creating this feature, but also writing the docs for it @valeriangalliat! |
This PR was merged into the master branch. Discussion ---------- [DomCrawler] Document images crawler | Q | A | ------------- | --- | Doc fix? | no | New docs? | yes (symfony/symfony#13649, symfony/symfony#13650) | Applies to | 3.0.0 | Fixed tickets | symfony/symfony#12429 Commits ------- 355a83f [DomCrawler] Document images crawler