ansonphong / postworld

Wordpress Theme Development Framework
GNU General Public License v2.0
7 stars 0 forks source link

load-feed Implementation // Override Attributes #68

Closed ansonphong closed 8 years ago

ansonphong commented 10 years ago

Hi Michel, So I'm in the process of implementing load-feed directive, and it's working generally quite well. There are a couple of issues which I'm facing right now, I'll document them below.

So here is a drawing of the implementation:

load-feed-2-sections


Overview

From the illustration you will notice several things:

  1. Both feeds are sharing the same feed ID, so are drawing off the same cached feed outline.
  2. Feeds have differing values for max_posts, feed_template, view.current and offset
  3. The top "Highlights Feed" requires no infinite scrolling.

So here are the issues in implementation.


Override Attributes

This would be the code view:

  feed_settings['features-front_page'] = {
    preload : 5,
    load_increment : 5,
    offset: 0,
    max_posts:200,
    view : {
      current : 'detail',
      options : ['grid','detail']
    },
    feed_template: 'features-front_page', 
  };
<!-- 3 HIGHLIGHTS FEED -->
<div load-feed="features-front_page" ng-include="templateUrl" max-posts="3" preload="3" view-current="grid" feed-template="hilight-front_page"></div>
<!-- SCROLLING FEED -->
<div load-feed="features-front_page" ng-include="templateUrl" offset="3"></div>

What do you think about that? Do you think that using override attributes is the way to solve this?

michelhabib commented 10 years ago

Let me throw a quick idea, before getting further in the details. If we put the feed ID [for which the outline is fetched from the database] as one of the attributes of the feed_settings object, and use a different id in feed settings, like feed_settings['features-front_page-scrolling'] and feed_settings['features-front_page-no-scrolling'], and then use whatever settings needed for each feed separately, does that make sense?

feed_settings['features-front_page-with-scrolling'] = {
    feed_id : 'features-front_page'
    // some settings here
    feed_template: 'features-front_page-with-scrolling', 
  };
feed_settings['features-front_page-no-scrolling'] = {
    feed_id : 'features-front_page'
    // some different settings here
    feed_template: 'features-front_page-no-scrolling', 
  };
ansonphong commented 10 years ago

Yes we can do that, no problem.

ansonphong commented 10 years ago

Actually the method you proposed here is better - let's go with that. Yes I agree. More elegant.

michelhabib commented 10 years ago

And faster to implement, check the latest commit now :) Kindly note that old implementation still works, so the code will check if you have a feed_id attribute in your settings, it will use it, if not, it will just use the feed_settings id as it used to be, old implementation will continue to work correctly. i didn't include a real example in the code, but if you use the sample above that i included, it should work fine.

ansonphong commented 10 years ago

Perfect!

On Thu, Oct 31, 2013 at 5:39 PM, michelhabib notifications@github.comwrote:

And faster to implement, check the latest commit now :) Kindly note that old implementation still works, so the code will check if you have a feed_id attribute in your settings, it will use it, if not, it will just use the feed_settings id as it used to be, old implementation will continue to work correctly. i didn't include a real example in the code, but if you use the sample above that i included, it should work fine.

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/68#issuecomment-27540648 .

ansonphong commented 10 years ago

One thing I noticed which relates to this, is the max_posts attribute in the feed_settings for load-feed doesn't seem to work, and it's using the settings of the registered feed.

Although I just tested and max_posts works correctly for live-feed

michelhabib commented 10 years ago

you are right, in load-feed, i am using all cached ids returned in feed_outline and ignoring max_posts. I am changing that behavior to make max_posts override it.

michelhabib commented 10 years ago

committed, max_posts is now respected in load-feed, it was previously overriden by total count of records in feed_outline.

ansonphong commented 10 years ago

excellent.

On Thu, Oct 31, 2013 at 7:24 PM, michelhabib notifications@github.comwrote:

committed, max_posts is now respected in load-feed, it was previously overriden by total count of records in feed_outline.

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/68#issuecomment-27543950 .

ansonphong commented 10 years ago

Working great.

ansonphong commented 10 years ago

I think this is the final issue with the load-feed for now - whereby I can't get the offset parameter to work. For instance, the illustration is the same as what was posted in the first message - though I have the Scrolling Feed and the 3 Highlights Feed both starting off with the same 3 posts, since they're both sampling from the same outline.

feed_settings['features-front_page-scrolling'] = {
      feed_id: 'features-front_page',
      offset: 3, // << This is not working
       ...
};

Setting offset:3 should skip over retrieving the first 3 posts in the array. Or else (to simplify), if it's already retrieved, it should skip over displaying them.

michelhabib commented 10 years ago

Phong, you are right, the offset field is not used in load_feed, there are 2 cases:- case 1, when registering a feed and using it, pw_load_feed does not take offset as a parameter. so i guess this needs to be updated in php then make sure the offset is passed to the function. case 2, when using a cached outline, are you sure you want to use an offset in that case? if you manually enter the list of post ids i mean?

ansonphong commented 10 years ago

Offset in the angular feed and the php function are different matters, as illustrated previously on the front-page feed. For instance, if you look now at : http://staging.realitysandwich.com/ .

For instance, on the left-column there are two load-feeds which should be the extension of the same cached feed outline. Instead, you see that the first 3 posts repeat here. This is the perfect use for the 'offset' parameter to be embedded in the angular feed controller. How easy is that to accomplish?

On Sun, Nov 10, 2013 at 6:35 AM, michelhabib notifications@github.comwrote:

Phong, you are right, the offset field is not used in load_feed, there are 2 cases:- case 1, when registering a feed and using it, pw_load_feed does not take offset as a parameter. so i guess this needs to be updated in php then make sure the offset is passed to the function. case 2, when using a cached outline, are you sure you want to use an offset in that case? if you manually enter the list of post ids i mean?

— Reply to this email directly or view it on GitHubhttps://github.com/phongmedia/postworld/issues/68#issuecomment-28151692 .

ansonphong commented 10 years ago

Does this issue make sense with offset?

We don't want to pass 'offset' to PHP, we just want to condition the result on the client-side. To make it easy, you can even load the first 3 posts, that's fine, just we don't want to display them.

You will see currently on http://staging.realitysandwich.com/ on the left column, how the top 3 posts repeat across the two load-feeds - I'd like to just add offset:3 to the lower one.

michelhabib commented 10 years ago

understood, working on it now.

michelhabib commented 10 years ago

updated in latest commit - just use the offset and it will work now.

Note that now i slice the initially loaded posts/outline by offset, just to keep everything working well, like ads and load increment and so on. note that you just need to preload additional posts, so if offset is 3 and preload is 10, it will preload 7. put now preload = 13, it will preload 10. i can change that in the code, but i need first to study all dependences.

ansonphong commented 10 years ago

Ok that makes sense. That's ok for now with the preload offset, though let's keep a note of that to fix that in a future version later.

On 2013-11-20, at 4:44 AM, michelhabib notifications@github.com wrote:

updated in latest commit - just use the offset and it will work now.

Note that now i slice the initially loaded posts/outline by offset, just to keep everything working well, like ads and load increment and so on. note that you just need to preload additional posts, so if offset is 3 and preload is 10, it will preload 7. put now preload = 13, it will preload 10. i can change that in the code, but i need first to study all dependences.

— Reply to this email directly or view it on GitHub.