Skip to content

Conversation

@matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Jun 7, 2016

Fixes #36

@PBruk
Copy link
Contributor

PBruk commented Jun 7, 2016

Tagging @matt-bernhardt for code review: Fixes #36

@matt-bernhardt
Copy link
Member Author

matt-bernhardt commented Jun 7, 2016

A few things:

  • Can you add some conditional checks or scope these functions in some way? Right now, my concern is that these will add empty tags to feed items that don't have these fields populated.
  • the_content_rss has been deprecated in favor of the_content_feed per https://codex.wordpress.org/Template_Tags/the_content_rss - can you switch this to use the newer function?
  • Line 255 needs to be split up into multiple lines, and the second part (an echo statement inside a conditional) seems like it may be debugging-related? The function returns a $content variable, so I'm not sure why an echo is needed as well.

@PBruk
Copy link
Contributor

PBruk commented Jun 8, 2016

@matt-bernhardt, I made changes according to the comments you posted.

@matt-bernhardt
Copy link
Member Author

Hmm...looking at what's being produced on the test server it seems like this may need some more work.

The subtitle and perhaps the date field are being echoed in places that prevent the feed from validating. When I load the overall feed (/news/category/1/feed/) on the test server, I see the free text between the and content:encoded elements, which looks like the subtitle and a nonsense event date. This invalidates the feed, which may cause problems with some readers.

Event dates aren't being included where needed. Again on the test server, there is an event on 4/20/2015 about "everyone needs an octopus". When I locate that item in the feed at /news/events-feed/ I see no mention of that date.

@matt-bernhardt matt-bernhardt force-pushed the phillip/36_add_custom_fields_to_RSS branch from 9cbf44e to 09de4dd Compare June 8, 2016 18:51
@PBruk
Copy link
Contributor

PBruk commented Jun 10, 2016

@matt-bernhardt, I converted the jQuery date-picker field into PHP and this seems to have fixed the problem.

@matt-bernhardt matt-bernhardt added this to the Communiation Requests milestone Jul 21, 2016
@matt-bernhardt matt-bernhardt force-pushed the phillip/36_add_custom_fields_to_RSS branch 3 times, most recently from 3efa95d to 0bc2ffd Compare August 9, 2016 18:38
@matt-bernhardt matt-bernhardt force-pushed the phillip/36_add_custom_fields_to_RSS branch from 0bc2ffd to 75160ec Compare August 26, 2016 18:45
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.

2 participants