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

[UX] Provide an admin label and description for all blocks #4558

Closed
ghost opened this issue Aug 21, 2020 · 37 comments · Fixed by backdrop/backdrop#3267
Closed

[UX] Provide an admin label and description for all blocks #4558

ghost opened this issue Aug 21, 2020 · 37 comments · Fixed by backdrop/backdrop#3267

Comments

@ghost
Copy link

ghost commented Aug 21, 2020

Description of the need

If you create a custom block in a layout, but you don't want it to be reusable, there's no way to give that block an admin label that's not displayed on the frontend. Similarly, if you add lots of, say, hero blocks to a layout (all with different visibility conditions), there's no way to differentiate them in the layouts UI if you don't give them a frontend-visible block title.

Proposed solution

Just as custom, reusable blocks have admin labels that are seperate from frontend block titles, so should all other blocks have this ability. All blocks should get an optional 'Admin label' field that displays in admin interfaces only (not the frontend).

Additional information

This issue is a combination of #4325 (which requests this for existing, pre-made blocks) and #1703 (which requests this for non-reusable, custom blocks). Both have been marked duplicates of this new one.


PR by @docwilmot : backdrop/backdrop#3267

@docwilmot
Copy link
Contributor

Assigning to me

@ghost
Copy link
Author

ghost commented Aug 21, 2020

Assigning to me

When @docwilmot takes over an issue, you know it's going to be good! 😄

@Egarbeil
Copy link

I can't wait! It will help so much with site building. And I have a LOT of sites to convert over the next year or two.

@docwilmot
Copy link
Contributor

When @docwilmot takes over an issue, you know it's going to be good! 😄

Thats quite an endorsement there, thanks. I feel kind of obligated towards Layout issues since I helped with much of the early work on layouts, and so when things dont work and you dig deep enough, its probably my fault. 😄

@docwilmot
Copy link
Contributor

All blocks now have a field to set an administrative title.
admin title
Please test and milestone if OK.

@Egarbeil
Copy link

Egarbeil commented Aug 27, 2020 via email

@stpaultim
Copy link
Member

stpaultim commented Aug 27, 2020

This is awesome. Great work.

But, I think we have a problem. In a couple of situations we seem to have redundant options. It is now possible for a custom block to have:

  1. A display title (or custom title)
  2. An admin label (if it's reusable)
  3. And an administrative title

In a case like this, the admin label and administrative title seem redundant AND the help text for administrative title is wrong, because if the administrative title is left blank, the admin label is used.

Ideally, I think the option would have been to allow an admin label on blocks that are not reusable. Then we could have avoided the new concept of an administrative title. However, if there is a reason that this does not work. We may just need better help text or something to distinguish the admin label from the administrative title.

Home_page__home____PR_3267_backdrop_backdrop_testing_site

It might be ok to have all three options as long as there is a clear reason for it, we do our best to help users understand the difference and make sure our help text is consistent and clear.


Depending upon solution to the first issue, the title of the field set and the field title seem redundant. Do we need both?

Home_page__home____PR_3267_backdrop_backdrop_testing_site

My suggested change to help text may no longer make sense, given the other issues.


I think solving this problem is a really good thing, so I hope we can make this work.

@docwilmot
Copy link
Contributor

😄 I hadnt realised this about custom blocks. Will reassess.

@klonos klonos modified the milestone: 1.17.0 Aug 27, 2020
@klonos
Copy link
Member

klonos commented Aug 27, 2020

...ooops! I've jumped the gun there. We need an advocate for this issue in order to add a milestone; or get it to RTBC before September 1st.

https://backdropcms.org/user-guide/adding-milestones-to-issues

@klonos
Copy link
Member

klonos commented Aug 27, 2020

Thanks for working on this @docwilmot 🙏 ...comments:

  • I like @stpaultim's suggestion for the help text addition 👍

  • I believe that we use the term "label" for things that will be for admin-eyes-only, in the admin UI.

  • Perhaps this could work better as a checkbox under the block title, similar to how the description checkbox works for views:

    image

@Egarbeil
Copy link

Egarbeil commented Aug 27, 2020 via email

@docwilmot
Copy link
Contributor

I wanted to save the admin titles out of the way, since that would only be important in one type of block (BlockText) and only then in certain situations, hence hiding in an admin title (can rename to "admin label" for consistency) fieldset, like Views does.

I could disable the new fieldset on custom blocks and use the existing "Admin label" checkbox for those. Would that do?

@Egarbeil
Copy link

Egarbeil commented Aug 29, 2020 via email

@ghost
Copy link
Author

ghost commented Aug 29, 2020

I would prefer consistency across all blocks. So the admin label should look/work the same everywhere.

Guess this means either making the new ones match the existing ones, or changing the existing ones with the new location/field set.

@docwilmot
Copy link
Contributor

I pushed a commit that addresses this earlier. Need to rename some things and use the description and do upgrade path (and there's probably still some kinks) but likely will be tomorrow. But please check out the UI change and let me know if that's workable.

@klonos
Copy link
Member

klonos commented Aug 30, 2020

@docwilmot was, that works for me 👍 ...there are a few issues, but since you already mentioned that it's WIP, I won't go into details. There's definitely more consistency. Let us know when this needs another round of testing/review. Thanks.

@docwilmot
Copy link
Contributor

Because I didnt actually save the right change? 🙄

@stpaultim
Copy link
Member

stpaultim commented Sep 2, 2020

Works for me! Great work.

@klonos
Copy link
Member

klonos commented Sep 2, 2020

I have tested this and it works as expected. I think that it's good to be merged as is now, for 1.17.0, and then we have a couple of weeks ahead to polish things.

Polishing:

The fieldset needs a better label, and the "This text is used only in administrative interfaces." bit applies to both the admin label and the description, so perhaps add it on the fieldset instead? I was thinking something like:

Text used to identify the block when multiple blocks of the same type are added on the same layout. This is not shown to site visitors - only in administrative pages.

So it'll look like this:

image

Compare to current:

image

@klonos
Copy link
Member

klonos commented Sep 2, 2020

Reviewing code next...

@klonos
Copy link
Member

klonos commented Sep 2, 2020

OK, I think that this is good to go! ...great work indeed @docwilmot 👍 ...as always 🙂

@Egarbeil
Copy link

Egarbeil commented Sep 2, 2020 via email

@quicksketch
Copy link
Member

Looks good to me. I was trying to figure out if we could reduce code duplication, we have this same snippet in a dozen places:

    if (!empty($this->settings['admin_label'])) {
      return check_plain($this->settings['admin_label']);
    }

But even if we made a shared method for getting the admin label, it wouldn't be any shorter than these 3 lines.

I see one test is updated to use the admin_label value. Could we add a bit more test coverage to include checking admin_description?

the "This text is used only in administrative interfaces." bit applies to both the admin label and the description, so perhaps add it on the fieldset instead? I was thinking something like:

I actually like the description below each field separately, but that may be a personal problem that I skip over fieldset descriptions when reading but individual fields the description is right there so I read it while I'm typing and looking at the individual field.

Marking this needs work for the test coverage.

@docwilmot
Copy link
Contributor

But even if we made a shared method for getting the admin label, it wouldn't be any shorter than these 3 lines.

I debated this for at least 24 hours, even tried it. Thats what I concluded too.

Marking this needs work for the test coverage.

Done

@quicksketch
Copy link
Member

Thanks @docwilmot! I've merged backdrop/backdrop#3267 into 1.x for 1.17. Great work and an excellent turn-around on this feature. 👍

Thanks @BWPanda for filing the request. And thanks to @stpaultim, @klonos, and @Egarbeil for your reviews and feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants