Word filters#311
Conversation
Refactored isOnline in the UserPresenter.
euantorano
left a comment
euantorano
left a comment
There was a problem hiding this comment.
Looks good so far, just some minor comments on the current progress.
| * @var array | ||
| */ | ||
| protected $casts = [ | ||
| 'id' => 'int', |
There was a problem hiding this comment.
Indentation here is a little weird for some reason.
| 'replace' => 'string', | ||
| ]; | ||
|
|
||
| public function getID() : int |
There was a problem hiding this comment.
I prefer this styled as getId() personally, and I think our style guide says something similar.
|
|
||
| namespace MyBB\Core\Database\Repositories\Eloquent; | ||
|
|
||
| use MyBB\Core\Database\Repositories\Collection; |
There was a problem hiding this comment.
Group the two Repository classes together like:
use MyBB\Core\Database\Repositories\{
Collection,
WordFilterRepositoryInterface
};| $this->wordFilter = $wordFilter; | ||
| } | ||
|
|
||
| public function getAll() : \Illuminate\Database\Eloquent\Collection |
There was a problem hiding this comment.
I think there's a collection interface/contract, it's better to state that we return an interface rather than a concrete type.
| // TODO: Implement this | ||
| //public function find(int $id) : WordFilter; | ||
|
|
||
| public function getAll() : \Illuminate\Database\Eloquent\Collection; |
There was a problem hiding this comment.
Again, I think there's a collection interface/contract, it's better to state that we return an interface rather than a concrete type.
It's probably also best to have a paginate() method and paginate them in the ACP. If you have many filters, looking through them all is difficult.
| $this->wordFilterRepository = $wordFilterRepository; | ||
| } | ||
|
|
||
| public function index() : \Illuminate\View\View |
There was a problem hiding this comment.
Personally I prefer to return a response interface in the function signature, in case something changes in the future.
| */ | ||
| protected $guard; | ||
|
|
||
| public function __construct(WordFilterModel $resource, Guard $guard) { |
There was a problem hiding this comment.
{ should be on the line below.
| 'title' => 'Word Filters', | ||
| 'add_button' => 'Add Word Filter', | ||
| 'find_field' => 'Find', | ||
| 'replace_field' => 'Replacement', |
There was a problem hiding this comment.
I wouldn't bother trying to line all the values up, it just gets annoying trying to maintain it later down the line 😉
| </div> | ||
| </div> | ||
| <div class="admin-body__content" style="display: flex;flex-direction: row;"> | ||
| <table class="admin-table admin-table--bordered users" style="width:79%;"> |
There was a problem hiding this comment.
I mentioned to @Azareal in Discord, we will want to avoid inline style and tables. For now the classes already available on these elements should work fine for removing that inline stuff, the tables can be replaced later so I wouldn't worry about them at this moment. The class "users" should be replaced with something more appropriate word-filters.
| </tr> | ||
| </thead> | ||
| <tbody> | ||
| {% for filter in word_filter_items %} |
There was a problem hiding this comment.
Probably a good idea to use an else block to display a message if the collection is empty.
3 participants
Word Filter Component for the ACP and a little refactor for the isOnline method of the UserPresenter.
I've so far done the word filter list.