Move Discussion List State into its own class #2150
Conversation
askvortsov1
changed the title
| params.page = { offset }; | ||
| params.include = params.include.join(','); | ||
|
|
||
| return app.store.find('discussions', params); |
There was a problem hiding this comment.
Do we want to pass in the app (or store, or whatever dependencies there are) to the state as well? 🤔 That would make it easier to test them, potentially... 🤔
There was a problem hiding this comment.
Isn't that what's done here? App is globally available
There was a problem hiding this comment.
That's the opposite of passing it in. We're accessing a global variable, I was suggesting to do dependency injection.
There was a problem hiding this comment.
Redux, Meiosis, and most other state management methodologies are based around having a single, global state object. Now, typically the main reason for wanting to avoid globals is that anything, anywhere can modify them, leading to unintended consequences. That's where the main point of these methodologies comes in: the modification of the global state is controlled by the methodology. For instance, with Meiosis, http://meiosis.js.org/tutorial/01-introduction.html
There was a problem hiding this comment.
I understand that concept, and the states and stores being a central place makes sense. I was discussing the method of accessing this central place: by calling on a global name (app.store) or by passing it in to the constructor (dependency injection). The latter not only makes tests easier, but it also allows passing in a different object with the same interface, should the need ever arise (and part of our extensibility philosophy is not knowing what use-cases might come up 😆 ).
There was a problem hiding this comment.
Alright, I see your point here. I'll change it so that if app is needed by the states, it gets passed in on initialization (or should app.store be passed in?)
There was a problem hiding this comment.
After some more thought, I'm going to side with passing in the entire app. Pretty much every piece of the app could be reused in some way or another.
There was a problem hiding this comment.
Okay, thanks. No strong opinion there right now. We can revisit this when we have states everywhere, and see how it looks then / what patterns emerge / whether we want to refactor towards another solution.
There was a problem hiding this comment.
I definitely like where this is going already.
| @@ -37,7 +38,7 @@ export default class DiscussionPage extends Page { | |||
| // page, then we don't want Mithril to redraw the whole page – if it did, | |||
| // then the pane would which would be slow and would cause problems with | |||
| // event handlers. | |||
| if (app.cache.discussionList) { | |||
| if (app.cache.discussionList.hasDiscussions()) { | |||
There was a problem hiding this comment.
This seems like slightly different behavior - "the discussion list being loaded" (i.e. present) is a different condition from "the discussion list having discussions". An empty list can be loaded as well, right?
There was a problem hiding this comment.
I think this was brought up by David in another place before, so I might be misguided pointing this out again here.
There was a problem hiding this comment.
Correct, I brought it up @ #2150 (comment).
There was a problem hiding this comment.
Ah, right. This is the reverse condition, though, and here we now have an unhandled case of "results loaded, but none found"...
There was a problem hiding this comment.
I don't see any benefit to showing the slideout when there's no discussions?
…throughout and simply refresh its contents
| const text = app.translator.trans('core.forum.discussion_list.empty_text'); | ||
| return <div className="DiscussionList">{Placeholder.component({ text })}</div>; | ||
| } | ||
|
|
||
| return ( | ||
| <div className={'DiscussionList' + (this.props.params.q ? ' DiscussionList--searchResults' : '')}> | ||
| <div className={'DiscussionList' + (state.isSearchResults() ? ' DiscussionList--searchResults' : '')}> |
There was a problem hiding this comment.
hasSearchResults? I don't know how something is a search results.
There was a problem hiding this comment.
Perhaps searched? isSearching? isSearch? Either works yeah.
| @@ -49,6 +50,7 @@ export default class DiscussionPage extends Page { | |||
| app.history.push('discussion'); | |||
|
|
|||
| this.bodyClass = 'App--discussion'; | |||
| this.discussionListClass = DiscussionList; | |||
There was a problem hiding this comment.
That was a leftover an intermediate step which we decided to revert again. Good catch, removed in master.
|
Breaking changes:
|
Fixes Part Of #2144
Changes proposed in this pull request:
Reviewers should focus on:
Anything I missed?
Confirmed
composer test).EDIT THIS BEFORE MERGING: #2151 (review)