backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[UX] Layout/block UI: Better field block summaries. #3236

Open klonos opened 6 years ago

klonos commented 6 years ago

Describe your issue or idea

As it is currently, the description of any field block added to the layout has "Displays values of [field_name] field" as its description:

screen shot 2018-08-04 at 6 05 30 am

This does not give any other useful information, and in fact using plural "values" is misleading when the field has cardinality 1.


PR by @klonos: https://github.com/backdrop/backdrop/pull/2261

klonos commented 6 years ago

...so the current PR does the following:

Here's what it looks like:

screen shot 2018-08-04 at 6 03 44 am
opi commented 6 years ago

The double parenthesis looks strange...

klonos commented 6 years ago

The double parenthesis looks strange...

I know, but it is coming from the default field type description in https://github.com/backdrop/backdrop/blob/1.x/core/modules/field/modules/text/text.module#L38. What do you propose we do instead?

Comment by @opi on Gitter:

what about removing the "Displays the" prefix ? seems pretty useless to me

I was going for natural language. I see now that some block descriptions use statements like "The default content of this page." or "The current page breadcrumb trail." or "A hierarchical list of links..." etc., but we then also have descriptions that use proper phrases, starting with verbs, like "Contains elements typical of...". and "Displays a link to ...". I don't mind either; I was just keeping what was already there.

opi commented 6 years ago

OK for this natural language thing, as I'm not that fluent in english, my opinion doesn't worth so much.

For the field type display, I made a test to print it in the summary part:

klonos commented 6 years ago

I was going for a simple PR/change, while adding setting labels in the summary will mean updating the _field_formatter_settings_summary hooks for all field types. Printing the field type along with the summary is not a bad idea though @opi 👍...here's how this would look with the labels of settings bolded (for better readability):

screen shot 2018-08-05 at 6 52 05 am

I will file a separate PR so people can decide.

opi commented 6 years ago

Updating hook_field_formatter_settings_summary will also affect the content type display setting page, which is not what we want (maybe I'm wrong). Field type could be added into summary from the FieldBlock->getAdminPreview() method.

Also, bolding the label is nice, but will affect all existing translations... but why not.

klonos commented 6 years ago

Updating hook_field_formatter_settings_summary will also affect the content type display setting page ... Also, bolding the label is nice, but will affect all existing translations... but why not.

Yeah, I have thought of that, but I like uniformity/consistency, so I like how this looks:

screen shot 2018-08-06 at 5 45 32 am

I would like to get some feedback from others too before spending time on a new PR.

olafgrabienski commented 6 years ago

... will also affect the content type display setting page ... Also, bolding the label is nice ...

The bold labels of the field settings on the content type Manage Displays page seem too strong to me because -- in contrast to the Layout UI -- there the field names itself are not bold.

what about removing the "Displays the" prefix ?

I was going for natural language. I see now that some block descriptions use statements like "The default content of this page."

Yes, looks pretty inconsistent right now. In my opinion, repeated prefixes like Displays the are sort of distracting: they make it more difficult to scan the descriptions because all of them start with the same words. However, in this case of fields in Layouts, why don't we omit the description text if it "does not give any other useful information", and only display the field setting values?

herbdool commented 6 years ago

I'll just comment on one part: no need to insert "the". It sounds just as natural without the "the" because of the context. It's not unusual to omit the word on signs and messages since the space is limited and need to convey info quickly.

quicksketch commented 6 years ago

Did @opi file an alternative PR? Looks like it needs cross-linking (or better, put it in the original issue description).

+1 to comments by herbdool and olaf above.

opi commented 6 years ago

I do not filed another PR. My screenshot was just a quick'n'dirty local test.

klonos commented 6 years ago

@quicksketch so just to clarify the "needs work" bit, from the comments above I am assuming the following:

  1. no bold labels for properties
  2. remove the "Displays the" prefix

...anything else?