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

Feeds plugins: CORS enabled pulling #8502

Merged
merged 1 commit into from Dec 13, 2018
Merged

Conversation

ipaksc
Copy link
Contributor

@ipaksc ipaksc commented Oct 29, 2018

No description provided.

@LaurentGoderre
Copy link
Member

@ipaksc it's counter to best practices to add the date to a commit message. It makes the commit very verbose and duplicates info that is already available with the commit.

@ipaksc ipaksc force-pushed the rsscors branch 7 times, most recently from 180b60f to c048446 Compare October 29, 2018 19:17
@ipaksc ipaksc changed the title 29.10.2018: CORS enabled pulling Feeds plugins: CORS enabled pulling Oct 30, 2018
@duboisp
Copy link
Member

duboisp commented Nov 7, 2018

Before to merge, I am going to test this PR locally first.

@duboisp
Copy link
Member

duboisp commented Nov 21, 2018

@@ -15,6 +15,27 @@

<section>
<h2>Examples</h2>
<section class="wb-feeds limit-3" data-cors="true">
<h3>Canada News Centre - Manitoba</h3>
Copy link
Member

Choose a reason for hiding this comment

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

Can you modify the title to indication this example use the CORS?

Copy link
Member

Choose a reason for hiding this comment

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

Add also a small description explaining: This method fetch the ATOM feed directly and don't use a third party service.

Choose a reason for hiding this comment

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

Great

@@ -15,6 +15,27 @@

<section>
<h2>Exemples</h2>
<section class="wb-feeds limit-3" data-cors="true">
<h3>Centre des nouvelles du Canada - Manitoba</h3>
Copy link
Member

Choose a reason for hiding this comment

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

Can you modify the title to indication this example use the CORS?

@fibrahim
Copy link

What is the net effect of adding data-cors="true" to the mark-up? Is the Yahoo API sill being used?

@duboisp
Copy link
Member

duboisp commented Nov 21, 2018

@fibrahim The yahoo API should not be used.

@duboisp
Copy link
Member

duboisp commented Nov 21, 2018

I updated the test site as per the latest Ilya changes.

Now the CORS example is the last working example.

Test URL: http://universallabs.org/wet/labs/pr-8502/demos/feeds/feeds-en.html
WET-BOEW build with this fix: http://universallabs.org/wet/labs/pr-8502.zip

Copy link
Member

@duboisp duboisp left a comment

Choose a reason for hiding this comment

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

The working example don't work. It seems to conflict with feed that are fetch with the Yahoo API.

@ipaksc ipaksc force-pushed the rsscors branch 2 times, most recently from f6f0486 to bcb2c67 Compare November 23, 2018 14:52
if ( !Array.isArray( data ) ) {
data = [ data ];
// if CORS -> transform the xml response into a JSON
if ( $( eventTarget ).parentsUntil( selector ).parent().data( "cors" ) === true ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can you cache this $( eventTarget ).parentsUntil( selector ).parent() into a variable.

@duboisp
Copy link
Member

duboisp commented Nov 29, 2018

I updated the test site as per the latest Ilya changes.

Test URL: http://universallabs.org/wet/labs/pr-8502/demos/feeds/feeds-en.html
WET-BOEW build with this fix: http://universallabs.org/wet/labs/pr-8502.zip

@duboisp
Copy link
Member

duboisp commented Dec 4, 2018

I updated the test site as per the latest Ilya changes.

Test URL: http://universallabs.org/wet/labs/pr-8502/demos/feeds/feeds-en.html
WET-BOEW build with this fix: http://universallabs.org/wet/labs/pr-8502.zip

* @method corsEntry
*/
corsEntry = function( xmlDoc, limit ) {
var arr_entry = [];
Copy link
Member

Choose a reason for hiding this comment

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

Do one "var" declaration instead of multiple, this is to follow the same coding style as we did elsewhere in WET. This apply to other "var" statement you added in your PR.

@ipaksc ipaksc force-pushed the rsscors branch 4 times, most recently from c23b50f to e27bb5a Compare December 6, 2018 15:47
@ipaksc ipaksc force-pushed the rsscors branch 3 times, most recently from 4c6d783 to bf59537 Compare December 6, 2018 16:07

if ( elm ) {
$elm = $( elm );
$content = $( elm ).find( ".feeds-cont" );
Copy link
Member

Choose a reason for hiding this comment

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

Can you replace ````$( elm )by$elm```?

w# with '#' will be ignored, and an empty message aborts the commit.
@duboisp
Copy link
Member

duboisp commented Dec 13, 2018

I updated the test site as per the latest Ilya changes.

Test URL: http://universallabs.org/wet/labs/pr-8502/demos/feeds/feeds-en.html
WET-BOEW build with this fix: http://universallabs.org/wet/labs/pr-8502.zip

@duboisp duboisp merged commit 2c5fdc3 into wet-boew:master Dec 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants