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

Make Query Loop settings more intuitive with a ToggleGroup and simplified help text #63739

Merged

Conversation

kmanijak
Copy link
Contributor

@kmanijak kmanijak commented Jul 19, 2024

What?

Fixes #63598.
Swap "Inherit query from template" toggle with Contents: Default | Custom toggle group.

What Before After
inherit: true image CleanShot 2024-07-26 at 09 23 00
inherit: false image CleanShot 2024-07-26 at 09 24 25

Why?

This control has confused users (and engineers) for a long time now. This is an attempt to make it clearer as per #63598.

How?

Replace ToggleControl with ToggleGroupControl component.

Testing Instructions

  1. Go to Editor and All Archives template
  2. Focus on Query Loop block
  3. Expected: There's "Contents" control as per screenshots above
  4. Switch to "Custom"
  5. Expected: All the customization controls appear
  6. Switch back to "Default"
  7. Expected: All the customization controls disappear and help text appears

Testing Instructions for Keyboard

Screenshots or screencast

Copy link
Contributor

@t-hamano t-hamano left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

I've provided some feedback on the code side, but overall it's working as expected.

As for the copy, let's continue to discuss what would be ideal.

richtabor

This comment was marked as outdated.

@richtabor
Copy link
Member

We should also make the post type help text much simpler:

"Select the type of content to display: posts, pages, or custom post types."

@kmanijak
Copy link
Contributor Author

Thanks @t-hamano and @richtabor! I fixed a small code improvement suggestion but haven't changed anything regarding the copy yet.

Let's continue the discussion in #63598 and keep it as a single source of truth. Once everyone agrees there I'll apply the final version here and set the PR to review 🙌

@jameskoster
Copy link
Contributor

Seems there's some consensus around this suggestion: #63598 (comment)

@kmanijak
Copy link
Contributor Author

Seems there's some consensus around this suggestion: #63598 (comment)

@jameskoster , I don't feel like everyone agreed with this one yet 🤔

@jameskoster
Copy link
Contributor

jameskoster commented Jul 26, 2024

It feels like the best suggestion so far. Maybe it's worth updating the PR to give it a try? Sometimes it helps to interact with the control in-situ.

It's easy to change if it doesn't feel right :)

@kmanijak
Copy link
Contributor Author

@jameskoster, yeah, makes sense 😅 pushed the changes, and here's how it looks now:

queryloopinspectorcontrols.mov

@jameskoster
Copy link
Contributor

What do you think @richtabor ?

I noticed some feedback here too: #31158 (comment). Specifically for this control, "Use template’s content selection" was suggested for the default selection.

@richtabor
Copy link
Member

Use template’s content selection

I don't think this particular help text assists you with understanding. What's a content selection?

Copy link
Member

@richtabor richtabor left a comment

Choose a reason for hiding this comment

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

It's close. Small edits, and a small suggestion for code simplification. I think it'd be fine to land, then revise if needed further. It's certainly an improvement from trunk.

@richtabor richtabor marked this pull request as ready for review July 26, 2024 13:20
@richtabor richtabor requested a review from ajitbohra as a code owner July 26, 2024 13:20
Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: kmanijak <karolmanijak@git.wordpress.org>
Co-authored-by: richtabor <richtabor@git.wordpress.org>
Co-authored-by: t-hamano <wildworks@git.wordpress.org>
Co-authored-by: jameskoster <jameskoster@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@richtabor richtabor added [Type] Enhancement A suggestion for improvement. [Block] Query Loop Affects the Query Loop Block labels Jul 26, 2024
@richtabor richtabor changed the title Revamp "Inherit query from template" into ToggleGroup allowing to choose between Default and Custom content Make Query Loop settings more intuitive with a ToggleGroup and simplified help text Jul 26, 2024
Copy link
Contributor

@jameskoster jameskoster left a comment

Choose a reason for hiding this comment

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

I think it'd be fine to land, then revise if needed further. It's certainly an improvement from trunk.

Agreed!

@richtabor richtabor enabled auto-merge (squash) July 26, 2024 13:54
@richtabor richtabor merged commit 062efa7 into WordPress:trunk Jul 26, 2024
67 of 71 checks passed
@github-actions github-actions bot added this to the Gutenberg 19.0 milestone Jul 26, 2024
@paaljoachim
Copy link
Contributor

Looks good! Thank you @kmanijak Karol!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Query Loop Affects the Query Loop Block [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

'Inherit query from template' setting is confusing
5 participants