Social Icons Widget: Add more common feed url components to supported icons #13183
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
As an improvement based on the issue Automattic#10662 "Social Icons Widget: allow for alternate feed URLs" this adds some more common feed url components to the get_supported_icons() function. The existing version only matches the existence of "/feed/" in the url being added, as such many other feed URL formats would end up showing with the default link icon instead. Whilst it may still not catch all possilbe feed url formats it will catch many commonly used formats as well as non-permalink versions of WordPress feeds.
|
Thank you for the great PR description! When this PR is ready for review, please apply the Scheduled Jetpack release: September 3, 2019. |
jeherve
added
[Pri] Low
[Status] Needs Review
jeherve
requested changes
Aug 5, 2019
jeherve
added
[Status] Needs Author Reply
jeherve
changed the title
Fixed incorrect use of spaces instead of tabs. Added some additional comments regarding which sources RSS feeds may come from.
There are many possible RSS Feed url formats that can be used to match for the RSS Feed icon, so to reduce repetition of multiple 'feed' entries in the `$social_links_icons` array a separate `$feed_url_formats` now contains the common 'feed' icon and the various feed url parameters. As such it means the 'url' of an item in the `$social_links_icons` array can itself be an array so the code for rendering the widget has been updated to accommodate this.
jeherve
requested changes
Aug 6, 2019
The previous commit added the various `$social_links_icons` 'feed' items as an array. This cleaned up the code but also added a bit of repetition due to having to check whether a `$social_links_icons` url element was an array and subsequently handle them in two different ways but duplicating some code in the process. This update changes all url elements in the `$social_links_icons` array to be an array even if there is only one item (as most of them do). This means that the code used to check if a url matches only needs to presume the url element comes as an array. This also enables the possibilty that some of the existing items e.g. Amazon can be reduced to one entry in the `$social_links_icons` array.
…cons` array This commit reduces the separate entries that exist for Amazon, Google, Discord and WordPress domains into single items with multiple url array items instead.
Now that all supported services use the array format, we can move this whole array back in without creating any confusion.
- Since we know for sure $social_icon['url'] is going to be an array, let's remove the extra check - Use printf instead of 2 echo - Add phpcs:ignore to avoid error on output
jeherve
added
[Status] Ready to Merge
jeherve
previously approved these changes
Aug 7, 2019
I had wondered about putting the 'feeds' array back into the main |
|
Caution: This PR has changes that must be merged to WordPress.com |
mdawaffe
reviewed
Aug 16, 2019
|
rickcurran, Your synced wpcom patch D31302-code has been updated. |
matticbot
added
[Status] Needs Changelog
and removed
[Status] Ready to Merge
jeherve
added a commit
that referenced
this pull request
Aug 27, 2019
* 7.7 changelog: initial set of changes * Changelog: add #13154 * Changelog: add #13134 * Changelog: add #12699 and many others * Changelog: add #13127 * Changelog: add #13167 * Changelog: add #13225 * Changelog: add #13179 * Changelog: add #13173 * Changelog: add #13232 * Changelog: add #13137 * Changelog: add #13172 * Changelog: add #13182 * Changelog: add #13200 * Changelog: add #13244 * Changelog: add #13267 * Changelog: add #13204 * changelog: add #13205 * Changelog: add #13183 * Changelog: add #13278 * Changelog: add #13162 * Changelog: add #13268 * Changelog: add #13286 * Changelog: add #13273 * Changelog: add #12474 * Changelog: add #13085 * Changelog: add #13266 * Changelog: add #13306 * Changelog: add #13311 * Changelog: add #13302 * Changelog: add #13196 * Changelog: add #12733 * Changelog: add #13261 * Changelog: add #13322 * Changelog: add #13333 * Changelog: add #13335
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As an improvement based on the issue #10662 "Social Icons Widget: allow for alternate feed URLs" this adds some more common feed url components to the get_supported_icons() function. The existing version only matches the existence of "/feed/" in the url being added, as such many other feed URL formats would end up showing with the default link icon instead. Whilst it may still not catch all possible feed url formats it will catch many commonly used formats as well as non-permalink versions of WordPress feeds.
This feature could be tested by beta testers to see if more feed urls are correctly recognised and shown with the RSS feed icon instead of the generic link icon.
Fixes #10662
Changes proposed in this Pull Request:
$social_links_iconswhich is used to match components of any url added to the Social Icons widget, the additions expand upon the existing/feed/array item which is intended to show an RSS feed icon if matched. Whilst this works for the standard WordPress permalink feed format it doesn't work for non-permalink WordPress feed urls and also many other common RSS feed url formats.Is this a new feature or does it add/remove features to an existing part of Jetpack?
get_supported_icons()function which matches urls added to the Social Icons widget to specific icons for the URL. This addition provides more items to the$social_links_iconsarray which match other common RSS feed url structures.Testing instructions:
To test this please try adding additional RSS feed urls to the Social Icons widget, the standard WordPress url for
somedomain.com/feed/is already supported so try some more unusual formats such as those generated by other CMS or blogging platforms. Here are some suggested urls to try:If these are correctly parsed then you should see an RSS feed icon instead of the default 'chain link' icon.
Proposed changelog entry for your changes: