Add setting to allow social media view to be displayed on the Latest Topics view only #7
Conversation
|
@pmusaraj Thanks. Reviewing now... |
|
There is an issue with this setting @angusmcleod I am amending the PR now. |
|
@angusmcleod this should be ready, please review. |
| @@ -39,7 +39,6 @@ export default { | |||
|
|
|||
| setupListStyle: function() { | |||
| if (this.get('socialMediaStyle')) { | |||
| this.set('skipHeader', true) | |||
There was a problem hiding this comment.
@pmusaraj Please make this contingent on your new setting being false rather than removing it.
| @@ -160,6 +160,15 @@ export default { | |||
| this.$('.topic-intro').prependTo(this.$('.main-link')) | |||
| this.$('.topic-title').prependTo(this.$('.main-link')) | |||
| } | |||
|
|
|||
| if (socialMediaStyle) { | |||
There was a problem hiding this comment.
@pmusaraj This should be in the TopicList component and should be confined to the necessary logic for your new setting. See the existing logic in the TopicList component here: https://github.com/pmusaraj/discourse-topic-previews/blob/a43842bd007711252f99605abc9981ae748c7000/assets/javascripts/discourse/initializers/preview-edits.js.es6#L42
There was a problem hiding this comment.
That's what I did initially, but the TopicList component only has a "didInsertElement" event, and it doesn't get reinserted when switching between latest and new (or unread, or a specific category). I struggled for a while to have this run inside TopicList, but then abandoned that option for the inefficient TopicListItem.
There was a problem hiding this comment.
@pmusaraj Did you try injecting the router into the component? I haven't tried something like this recently, but it may work.
| } else { | ||
| this.$().parents('#list-area').removeClass('social-media') | ||
| if (Discourse.SiteSettings.topic_list_social_media_only_latest) { | ||
| this.$('.topic-thumbnail').remove() |
There was a problem hiding this comment.
@pmusaraj Could you make this part a different setting? That will make this feature a little bit more usable by other folks as well. Also, please toggle the showThumbnail property rather than removing the element.
There was a problem hiding this comment.
Sure. So, as I understand it, the two options would be to have either a small thumbnail (not the same size as the social media layout) or no thumbnail.
There was a problem hiding this comment.
@pmusaraj hm, actually what this really calls for are filter-specific settings for all features, in addition to the existing category-specific settings. If you're in the mood, see if you can figure out the best way to handle toggling the existing feature set by filter in addition to toggling it by category. If not, just leave this in here for your setting and I'll work on that broader solution later. Please change the removal method to using showThumbnail though.
|
@pmusaraj Sorry I didn't realize you were done. See my comments. |
|
I think I figured out how to move the logic into the TopicList component, will refactor the PR (or delete, and create a new one). |
|
Ok, refactoring done. I added an observer for "topics" in several places, that seems to do the trick. It looks like it would be good to have filter-specific settings. I'm not sure how we can do it without overloading the settings interface though. Maybe instead of a "social media style" checkbox, we can have a list of the filters (latest, new, unread, top). But there is overlap between filters and categories, for example when going to I can work on this, but it will take a little while to get it all ready. Probably best as a separate PR. |
|
@pmusaraj Great I will review later today |
|
Added one more adjustment for mobile. Disabling social media for the other filters broke the display. |
|
@pmusaraj hm, that change forces social media layout for mobile web for all settings though. Not everyone uses social media layout. I'll add some updates here. |
|
True. I will open a GitHub issue for a refactoring of the plugin settings. I think between filters, categories and mobile/desktop, we need clearer settings. |
|
@pmusaraj Another issue is this exception that is being thrown when switching between latest (social media) and other lists. Perhaps because the list is being re-rendered by the observers in the topic list component and the element from the first render is removed (or never added), but the logic from the first render is still running. I'm adding in a check for
|
|
Moved here (so I could make commits): #11 |
No description provided.