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

Only mention “public” in post status option if relevant. #3

Merged
merged 3 commits into from
Dec 9, 2024

Conversation

rvdsteege
Copy link
Member

I found the post status setting options a bit overwhelming:

Scherm­afbeelding 2024-12-04 om 15 28 36

This PR changes the option to only denote "public" if that is the case for the post status:

Scherm­afbeelding 2024-12-04 om 15 36 45

Copy link
Member

@remcotolsma remcotolsma left a comment

Choose a reason for hiding this comment

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

I have struggled with this notation for a while. The disadvantage of this proposal is that if there are only 'public' => false post statuses, so no public post statuses at all, the visibility remains a guess for users. I have also tried to shorten this with emojis or something, but that was not necessarily very clear either. That is how I eventually came up with this notation, it is not perfect, but I doubt whether this PR is an improvement. The backtics are also more of a markdown thing, should we use it like that? We need custom selects → https://developer.chrome.com/blog/rfc-customizable-select, https://open-ui.org/components/customizableselect/ 😏.

@rvdsteege
Copy link
Member Author

Can we not use plain old <optgroup>? If there are no options for a group, the group can be omitted. If there are only options of a single group, the visibility is still mentioned/clear.

Scherm­afbeelding 2024-12-06 om 10 03 03

The backtics are also more of a markdown thing

I was somewhat hesitant to add them, but when both the post status and "public" were mentioned, I thought it needed some kind of distinction. However, the backticks are no longer needed with the optgroup and I've removed them in 63620a3.

Copy link
Member

@remcotolsma remcotolsma left a comment

Choose a reason for hiding this comment

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

Nice solution, smart to use <optgroup> elements for this.

Comment on lines 130 to 132
if ( 0 === count( $group['options'] ) ) {
continue;
}
Copy link
Member

Choose a reason for hiding this comment

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

I prefer to use https://www.php.net/array_filter so that there is no such logic in the foreach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed in 85d7052.

@rvdsteege rvdsteege requested a review from remcotolsma December 9, 2024 09:57
@rvdsteege rvdsteege merged commit 72ad6ac into main Dec 9, 2024
@rvdsteege rvdsteege deleted the post-status-option-clean branch December 9, 2024 10:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants