Use standard approach of displaying filters for list tables #5174
Conversation
|
Looks pretty good so far! One thing that I think could be improved a bit is the grading filters. Maybe remove the space between the inputs and have a filter button that is disabled by default and enabled once a lesson is selected? |
There was a problem hiding this comment.
Hey Dmitry, thanks for improving this part! 🙏
I have a few comments (mostly visual), so here it goes:
I noticed that filters started adding unnecessary params like _wpnonce and _wp_http_referer to the query. These params we add to the form, the same on the Posts page. But when I submit filters for Posts it doesn't add mentioned params to the query. Can't find how it is filtered there. Any ideas?
WP core is removing the parameters and making a redirect, but I'm not entirely sure why, to be honest. 🙂 Maybe it's a security issue? How hard would it be to do the same?
On mobile (chrome), the search input is missaligned.
The height of the select (select2) inputs is different than the button. I think it was like this before, but could it be easily fixed at least for the filters?
On mobile, the Grading filter inputs/button look small compared to the other inputs/buttons. The "Reset filter" button also has too much white space at the end.
Grading filters:
Other filters:
On mobile, the course filter on Reports -> Lessons is a bit off compared to the filter button.
If you feel some of the mentions are not related to the current PR, I'm OK with opening separate issues for them. 👍
| foreach ( $this->query_args as $name => $value ) { | ||
| if ( 'filter_by_course_id' === $name || 'filter_type' === $name ) { | ||
| continue; | ||
| } | ||
| $this->courses_select( $courses, $selected_course, 'courses-select-filter', 'filter_by_course_id', __( 'Filter By Course', 'sensei-lms' ) ); | ||
| ?> | ||
| <button type="submit" id="filt" class="button action"><?php echo esc_html__( 'Filter', 'sensei-lms' ); ?></button> | ||
| </div> | ||
| </form> | ||
| echo '<input type="hidden" name="' . esc_attr( $name ) . '" value="' . esc_attr( $value ) . '">'; | ||
| } |
There was a problem hiding this comment.
Hm, this causes input duplication now that we have one form.
I've also noticed the same issue on the other screens.
I don't think it's causing issues, so I'm also ok with leaving it as-is for now.
merkushin
force-pushed
the
update/usage-of-list-table
branch
from
e5a1fec
to
f2f3df5
Compare
Codecov Report
@@ Coverage Diff @@
## trunk #5174 +/- ##
============================================
+ Coverage 42.86% 44.56% +1.69%
- Complexity 8687 8728 +41
============================================
Files 412 412
Lines 31053 31115 +62
Branches 234 234
============================================
+ Hits 13311 13865 +554
+ Misses 17569 17077 -492
Partials 173 173
Continue to review full report at Codecov.
|
Haha, eventually, I know the answer. Yep, I think the main issue with these parameters is security. I suppose it is not safe to share your link with them, for example. I'm going to add the same redirect on our pages. |
# Conflicts: # includes/reports/overview/list-table/class-sensei-reports-overview-list-table-abstract.php
We can't redirect from these pages as output has already started. At the same time, we don't have POST forms and we can omit those fields.
|
@m1r0 Could you take a look again? @Imran92 Feel free to join if you have some time :) |
|
The changes look good! Just one more thing I've noticed is the search on the reports working funky. Maybe it's related to the inputs? Xu6i0h.mp4 |
|
Just reporting out to keep note: I found that this PR caused the issue on #5504, which, curiously, only happens when there are no items on the list page (in this case, for the students page). I'm still investigating the optimal way to fix this, but apparently, the root cause is that this PR opens a big Just sharing because it might be another thing to consider when doing those sorts of refactorings, especially when considering HTML forms. :) UPDATE: I sent PR #5583 to fix the new issue. |
In this PR I'm trying to display filters as it is done for Posts. It helps to avoid issues with positioning of elements. However, I found we have some special cases (the standard approach is to hide filters on mobile, but we have some required filters that we have to display always).
Known Issues
_wpnonceand_wp_http_refererto the query. These params we add to the form, the same on the Posts page. But when I submit filters for Posts it doesn't add mentioned params to the query. Can't find how it is filtered there. Any ideas? 🙏Changes proposed in this Pull Request
Testing instructions
trunk. There might be changes, but they should be "positive" (i.e. it should look like in Pages now instead of "custom" positioning).Screenshot / Video
After
Before
