Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix related section collapsing issue #145

Merged
merged 1 commit into from
Oct 24, 2022

Conversation

renintw
Copy link
Contributor

@renintw renintw commented Oct 19, 2022

Fixes #142

Originally, the Uses table in the Related section would be collapsed and show 0 records even though there are actually some records existing. After the fix, if the amount of records <= 5, it would never be collapsed.

Reason for changing from min to max

The return value of split_uses_by_frequent_funcs is infrequent_count, and the purpose of the function is to Rearrange the results of get_uses() so that frequent functions are pushed to the bottom. I'm not 100% sure why we want to show infrequently used functions to users first here (maybe I misunderstood the real purpose of the function), but according to the function's goal and its doc comment, I'm thinking that we can change from min to max here so that we can show as many infrequently used functions to users as possible.

Or if we still want it to show five records at most, we can specify the least records to show as well.
Perhaps:

$uses_to_show = min( $uses_to_show, max( split_uses_by_frequent_funcs( $uses->posts ), $least_uses_to_show) );

@renintw renintw self-assigned this Oct 19, 2022
@StevenDufresne
Copy link
Contributor

I can't speak to the particulars of frequency but the main goal was to avoid a very long list of functions.

@ryelle
Copy link
Contributor

ryelle commented Oct 21, 2022

I'm not 100% sure why we want to show infrequently used functions to users first here

You can see the original PR #37 — the purpose was to avoid always showing the same functions, ex __() or apply_filters for all functions. By showing rarely-used functions, the idea is that the functions shown are more relevant to the current page.

I think I prefer adding a min value to show (maybe 2?), rather than switching the min to max, that way we always hide after 5 items. /reference/functions/_wp_dashboard_recent_comments_row/ is a good test case with many rarely-used functions. On this PR, it shows 11 items before hiding the remaining 7.

@renintw renintw force-pushed the fix/related-section-incorrect-collapsing-behavior branch from b2ef277 to e25591f Compare October 24, 2022 16:33
Origainlly, Uses table in Related section would be collapsed
and show 0 record even there are actually some records existing.

After the fix, it would at least show 2 records.
@renintw renintw force-pushed the fix/related-section-incorrect-collapsing-behavior branch from e25591f to 3ed302a Compare October 24, 2022 16:37
@renintw
Copy link
Contributor Author

renintw commented Oct 24, 2022

I think I prefer adding a min value to show (maybe 2?), rather than switching the min to max, that way we always hide after 5 items. /reference/functions/_wp_dashboard_recent_comments_row/ is a good test case with many rarely-used functions. On this PR, it shows 11 items before hiding the remaining 7.

I prefer this as well. I've pushed a new commit for it, and in it I've removed the comment (ie the cutoff point for show/hide). as it's not used as the cutoff point. Thanks for sharing the test case!

@renintw renintw requested a review from ryelle October 24, 2022 16:40
@renintw renintw added the [Type] Bug Something isn't working label Oct 24, 2022
Copy link
Contributor

@ryelle ryelle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍🏻

@renintw renintw merged commit 0f8a03a into trunk Oct 24, 2022
@renintw renintw deleted the fix/related-section-incorrect-collapsing-behavior branch October 24, 2022 20:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Related section is collapsed on page load even though there only 2 uses.
3 participants