backdrop / backdrop-issues

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

Add the "Entity view mode" module to core #315

Closed jenlampton closed 7 years ago

jenlampton commented 10 years ago

This simple module provides a lightweight UI for adding additional view modes, and provides expected template suggestion hooks. https://www.drupal.org/project/entity_view_mode


PR by @docwilmot: https://github.com/backdrop/backdrop/pull/1645 initial port Latest PR by @jenlampton https://github.com/backdrop/backdrop/pull/1689

jenlampton commented 7 years ago

if it's not much to ask, can you please keep two separate PRs with the two different approaches in order to be able to try both in their respective sandbox to help decide which one feels like better UX?

Yes, that was what I was planning. I've already got 2 PRs, so I'll start a 3rd (see links in the parent issue)

jenlampton commented 7 years ago

I added the manage display drop-button link in this redux PR too, since I liked the idea so much :) screen shot 2016-12-10 at 8 03 44 pm

jenlampton commented 7 years ago

Okay, new PR at https://github.com/backdrop/backdrop/issues/1670 that removes the tabs for view-modes. Now on the landing page for "Manage displays" (changed from Manage display, singular) you see a list of view modes. The active ones have a link to "Manage display" as the first drop-button link. (I had to add a row so we could manage the default display, but no other changes were needed - all the menu items still work! No API change!)

This page only manages view mode info. Each display page only manages display info. It feels more sensible, and the code is cleaner now too :)

screen shot 2016-12-10 at 8 21 36 pm

P.S. Seeing it this way really makes me want to change the name View mode to Display Mode

klonos commented 7 years ago

My only concern with the approach in https://github.com/backdrop/backdrop/pull/1670 is that by making the "Manage display" tab a list of available (enabled/disabled) display modes, we are repurposing the entire UI: we are hiding something that users are used to be finding there before (the field hide/unhide/configure part) and are exposing something that was previously hidden away (the list of display modes) inside a fieldset for a good reason: people do not use it that much.

It's not that bad though. Here's a few thoughts/suggestions:

klonos commented 7 years ago

Something like this:

backdrop-issue315-display_modes_in_vtabs

docwilmot commented 7 years ago

@klonos nice idea. Main concern is performance: vtabs are just ordinary fieldsets after all, which means every field will be loaded in each vtab. If you have 20 fields and 10 vtabs, thats 200 fields on that page. Otherwise worth considering.

I agree with your concern about the repurposing of that page and users not finding it where it should be, but I dont think its that bad, coincidentally for the same reason: nobody used it much anyway. I think the existing UI is confusing, and this (with some help text) although a big change, would be an improvement.

One more thing with both your and @jenlampton's idea though, is the "enabled" checkbox, which I think may be unnecessary. All view modes are "enabled"; you can get an RSS view of a node right now, even with the checkbox unchecked. The custom settings checkbox was to highlight which view modes used custom settings vs the default setting. In the background Backdrop just adds this view mode to the entity bundle separately (using settings copied from the default view mode) to allow you to later save custom settings if you want to. Otherwise every view mode just uses the default.

So we could forgo the checkbox, and have a column like layouts and views that say "Default settings" or "Overridden". Every view mode then would have a dropbutton to edit display. Once you edit and save any view mode, we mark it as overridden. And of course a "revert" link. I think this would also be clearer than the current UI.

klonos commented 7 years ago

@klonos nice idea. Main concern is performance: vtabs are just ordinary fieldsets after all, which means every field will be loaded in each vtab. If you have 20 fields and 10 vtabs, thats 200 fields on that page. Otherwise worth considering.

Could we then load just the default vtab and "lazy-load" the contents of the rest vtabs in an AJAXy way as the user clicks through them? Would that be a solution to the performance issue?

...I dont think its that bad, coincidentally for the same reason: nobody used it much anyway.

Perhaps for the same reason the performance hit won't be a concern either 😉

One more thing with both your and @jenlampton's idea though, is the "enabled" checkbox, which I think may be unnecessary. All view modes are "enabled"; ... So we could forgo the checkbox, and have a column like layouts and views that say "Default settings" or "Overridden".

You are correct. I was not aware of all this. The checkbox can go away and in the vtabs mockup, the "Default settings" or "Overridden" status could be added to the summary text.

The only things that I have not added to my mockup (because I didn't know how well it would be accepted) are these:

I realize the performance concerns, but this is actually a UX pain with the current UI. If one needs to for example hide or change the display of a certain field in various display modes, they need to change it in each display mode and each time they need to click the save button before they move to the next display. If we did load all of them in a single form, then they would just need to click through the vtabs, make the change and click on the submit button only once (hence the "Save ALL display modes" button in my mockup 😉 ).

MrHaroldA commented 7 years ago

In my opinion; vertical tabs are for metadata of the form above. In the mock-up by @klonos I thought the form was missing ...

klonos commented 7 years ago

@MrHaroldA you seem to be right. The d.org UI Standards docs state the following:

Problem A lot of metadata settings are presented on a form, but the user does not need to see all those settings all the time. Solution Group subsets of related settings together and put them into vertical tabs, where each tab provides a short description of the most important information on that tab. One vertical tab is active at a time, displaying its settings. Use when

  • You have a group of form elements that are often ignored by the user
    • Metadata elements
    • Elements with good defaults ...

Recommendations

  • ...
  • Don't use it for the main interaction in the form
  • ...

Still, it felt like a good solution to our problem with this UI. The alternative would be to use a series of fieldsets (like we do in /admin/config/people/emails), but they have 2 downsides the way I see it:

In fact, just for fun, I tried converting the "Account emails" page to use vertical tabs instead of fieldsets and it seemed much better IMO.

Anyways, thanx for bringing this to our attention. I wouldn't have thought of it. I guess it's up for debate 😄

olafgrabienski commented 7 years ago

I like that the new approach in 1670 forces you to choose a certain view mode before editing it. That prevents you editing erroneously the Default mode just because it's visible by default while the once enabled Full content mode is out of sight.

Another aspect of the new approach may be confusing: When you visit or edit a view mode and want to go back to the Manage displays page, you have to use the breadcrumb link. To improve this situation, we could redirect to the Manage displays page after "Save" and add a "Cancel" link.

wesruv commented 7 years ago

I agree, with @MrHaroldA here https://github.com/backdrop/backdrop-issues/issues/315#issuecomment-266306140, this isn't an appropriate use of vertical tabs.

I like @jenlampton's screenshots (https://github.com/backdrop/backdrop-issues/issues/315#issuecomment-266261512), at least until we get further on Field UI redesign to clean this up more. Keep It Simple:tm: for now, trying to get this perfect in the current UI will cause more problems than it solves.

klonos commented 7 years ago

To be honest, I was expecting that. To my defense, the idea came to me as I was browsing the UI we have for "Enabled filters" section under /admin/config/content/formats/%format_name% where there's a section of checkboxes that when checked reveal a series of vtabs. I just thought, why not have the vtabs shown all the time instead?

But fair enough, it's going against well-thought UI guidelines and there's the performance concern too.

jenlampton commented 7 years ago

The operations dropbuttons lead users to a separate page. Hmm... In my head minimizing redirections and keeping things within context improves UX. This is doing the opposite.

This is actually the exact same sitemap architecture we have now, we just changed from using tabs for people to get there to using drop-buttons. (IMO an improvement, though a little one)

...or how about having these be vertical tabs instead

I don't like this idea at all, this would be improper use of vtabs, as stated by others above (and that other UI you were looking at @klonos is also guilty of improper use of vtabs - it's the only one in core that's "doin' it wrong" - but we do have another issue to fix up that UI too!)

.. the rows of the display modes that are disabled. Until they are enabled, they are just taking up space.

I agree, how about we sort the view-modes enabled-first, and color-code the disabled ones to be light gray to make them visually less prominent.

One more thing with both your and @jenlampton's idea though, is the "enabled" checkbox, which I think may be unnecessary. All view modes are "enabled"; you can get an RSS view of a node right now, even with the checkbox unchecked.

Thanks @docwilmot, this is an important point, I added the word Enabled but that's not quite the right word for this. How about we get rid of the checkbox entirely, and replace it with a drop-button action. We have those in other places in core, and I think this UI would make more sense if we did the same here - if only we could decide on the correct words.

How about we replace 'Enable' with Override and 'Disable' with Use default?

docwilmot commented 7 years ago

How about we replace 'Enable' with Override and 'Disable' with Use default or maybe Revert to default

Above suggestion:

So we could forgo the checkbox, and have a column like layouts and views that say "Default settings" or "Overridden". Every view mode then would have a dropbutton to edit display. Once you edit and save any view mode, we mark it as overridden. And of course a "revert" link.

We wont need an Override link then.

jenlampton commented 7 years ago

So we could forgo the checkbox, and have a column like layouts and views that say "Default settings" or "Overridden". Every view mode then would have a dropbutton to edit display. Once you edit and save any view mode, we mark it as overridden. And of course a "revert" link.

Ah, I see! So we remove the column we currently have for Storage state and replace it with a different column for Storage state that is talking about the state of the field settings, not of the view mode itself. I think this will make more sense for users. I'll give it a try.

jenlampton commented 7 years ago

Okay, we're on the way!

Here's a display that sorts the view-modes by active first, then using default. It colors the ones you can't "manage" yet a light gray.

I added a Status column for the active vs using default info, and removed storage-state since everyone seemed to agree that was confusing. I'm not sure about the word "Active" here, will keep thinking about that...

I removed the checkboxes on the left, and moved the Submit button into the last row, since all you do with it now is add a new view mode.

screen shot 2016-12-16 at 5 48 41 pm

I still need to figure out how to allow the using default modes to be changed to active.

I tried adding a "manage display" link for all view modes as @docwilmot recommended, but for those that are not yet active we get Access denied. I think if we're going to start changing how those callbacks work that might need to wait until 2.x (or maybe a follow-up?), so I'm going to try to add some kind of "Customize" vs "Revert to default" drop-button actions instead.

jenlampton commented 7 years ago

Getting closer, here I've added "Customize" and "Revert to default" drop-button actions for "Using default" and "Customized" displays. (I went with the word "Customized because we were using it before). screen shot 2016-12-16 at 7 32 50 pm

A few things I am still working on:

Could use feedback on Terminology:

jenlampton commented 7 years ago

Move disabled / add new view mode form into a collapsed fieldset.

I think I like this better:

screen shot 2016-12-16 at 9 13 38 pm

then expanded:

screen shot 2016-12-16 at 9 14 02 pm

klonos commented 7 years ago

Looking better and better @jenlampton 👍 ...

  • Move disabled / add new view mode form into a collapsed fieldset.

Not sure this will be necessary. Why hide a feature we're adding? The faded out color already deemphasizes their visual impact and the way it is now is how we do it in other places (Views list for example). I say it's good as is.

I want a confirm form for "Revert to default". That's a dangerous operation and people should be warned before we delete all their field display settings!

Yes. We have #769 for confirmation of node deletions, but I think that all confirmation pages should be modals. It's just a waste of page reloads otherwise.

jenlampton commented 7 years ago

Confirm form added, with helpful alert letting you know what you are about to delete: screen shot 2016-12-16 at 9 47 57 pm

Just pushed an updated PR to https://github.com/backdrop/backdrop/pull/1670

jenlampton commented 7 years ago

Not sure this will be necessary. Why hide a feature we're adding?

1) because of your comment above about making it harder for people to find the thing they need (Default) since there are now so many other options on the page. Let's take the most common use case: changing the display on the full-node view mode. When beginners come here, they will customize the Full content view mode by accident, instead of default, simply because we showed it to them. Hiding the stuff they don't need makes a better first-time user experience.

2) Because adding new view modes is not a majority use-case. We are adding the feature because it seemed like something that was missing from D7, but it's not something every site will need.

3) Because @quicksketch asked to see it that way (since that's how it was before)

4) and because after I saw it that way I started to like it better too, mostly because of 1, above :)

I think that all confirmation pages should be modals.

Totally agree! I think we have another issue to put all confirm forms into modals. (but that's outside the scope of this issue)

Do you have any thoughts on terminology? I think that's where I'm stuck :/

klonos commented 7 years ago

Fair enough. ...and the confirmation page looks good too.

Terminology-wise:

BTW, did we ever decide on "display modes" vs "view modes"?

Also, I was thinking that perhaps instead of "Additional" view modes, we could say "Non-customized"? This would make it self-explanatory why these are inside this fieldset and which condition makes it so that view modes are moved to/from it. Otherwise, it seems "magic" (and we might even get people requesting for a way to "manually" move things to/from it). If we did that though, we'd also need to move the add new view mode row outside the fieldset. So, all in all, lets not overthink or over-complicate things for now.

docwilmot commented 7 years ago

Move disabled / add new view mode form into a collapsed fieldset.

Totally disagree with this one.

The reason there was a fieldset in D7/Backdrop simply because that page tried to marry two separate concepts: field visibility and view mode management, and it was necessary to declutter the UI.

We have simplified that page to deal only with the management of View modes, so there is no need for this. There is no other UI (if we must talk consistency) where we bury inactive rows in a separate fieldset.

View modes are not ordered; we can float Default to the bottom and make it visually distinct. If Adding view modes is not a majority use case lets de-emphasize it with a local task link.

But that fieldset, is not a good idea.

jenlampton commented 7 years ago

Please see comment above about first-time experience.

The fieldset in Drupal 7 was not put there to separate concepts, it was to prevent beginners from using things they don't need to use.

We make that problem worse here by exposing the Full content view mode in this UI. We're going to see an increase in beginner mistakes unless we can do something to hide those actions better. My guess was a fieldset, but we could remove the rows from this list entirely, or pursue other options.

docwilmot commented 7 years ago

Well, no, thats inaccurate. See the whole discussion on the birth of that page here for reference: https://www.drupal.org/node/553298.

Realize that the current UI isnt preventing 'exposing' the Full content view mode. There is no 'mistake' in editing a view mode. In either case to actually alter a view mode requires you first hit "Customize" then still change the field displays. And if you change your mind there's a big ole 'Revert' link.

Also, aside, in all of the view-mode-table-on-the-display-page ideas, we need some way to emphasize "Default" as the source for settings for other view modes and not an actual view mode.

jenlampton commented 7 years ago

Also, aside, in all of the view-mode-table-on-the-display-page ideas, we need some way to emphasize "Default" as the source for settings for other view modes and not an actual view mode.

Yes, I agree. I made it bold on the manage displays page in hopes of drawing attention to it, but I think we could use some help text at the top of the page too.

olafgrabienski commented 7 years ago

I think we could use some help text!

Yes, please!

Unfortunately, I'm not sure about Default and Full content myself.

For example: If Default "is not an actual view mode" - @docwilmot - it behaves however like a view mode as long as you don't use Full content, doesn't it?

docwilmot commented 7 years ago

@olafgrabienski a node can be viewed either as Teaser, or Full, or RSS, etc, but there is no way to view a node as "Default". All of those view modes however will have the exact same settings (because they all are just really "copies" of Default) unless they are 'overridden' or 'customized'. Default is not a View Mode.

This is mostly for convenience, so that all new View modes will automatically inherit proper settings for all fields without having to set each field over each time. If you remove the customized settings on a view mode it falls back to inheriting from Default.

So there really is no issue with changing the settings on the Full view mode or any view mode. Youll only be changing that one, and you can reset it with just a click.

olafgrabienski commented 7 years ago

@docwilmot Thanks for your feedback, I see! Reminds the 'Master' display in Views.

In my opinion, the concept Default vs. actual view modes remains a bit unclear though: We're used to see Default and Teaser on a not yet customized content type under Manage display, because Teaser gets enabled by default. So, if you don't need other view modes than Teaser, it happens easyly to ignore other view modes and to just use Default for the display of full nodes.

To make things more clear, I'd suggest to enable the Full content view mode by default as well.

jenlampton commented 7 years ago

To make things more clear, I'd suggest to enable the Full content view mode by default as well.

This is not needed on 90% + of sites, and creates extra work for admins. With it enabled, a single field will need to have it's display configured in 3 places (default, teaser, full) instead of two (default, teaser). I'd hate to increase work for 100% of backdrop site admins by enabling a feature that is used by so few.

I think for Backdrop 2.x we could revisit the idea of actually disabling all view modes until defined here, but that's too much of an API change for Backdrop 1.x.

olafgrabienski commented 7 years ago

Hm, but would extra work even be necessary? When Full content is used, there is no need to configure anything on Default. Or am I overlooking something?

jenlampton commented 7 years ago

When Full content is used, there is no need to configure anything on Default. Or am I overlooking something?

You are overlooking every other view mode: RSS, Search result, Search index, etc, etc, etc. Any contrib module can add more. Default is always needed. It's a tool that allows things to be rendered even when an admin has not specifically defined fields for each view mode individually.

docwilmot commented 7 years ago

@olafgrabienski maybe youre misunderstanding the concept of "enabling" a view mode. As in my comment above all view modes are enabled. The ones which appear on the display page (teaser for example) are only the ones which are overridden (ie, not using default, ie using custom settings).

If you wanted to hide labels for body fields on all view modes, you would currently only need to change this for default (which would cause all other view modes including Full to inherit this hidden label) and you would need to hide the label separately for the teaser view mode (because it doesnt inherit from Default anymore, since its using custom settings).

So in other words, the more view modes we ship Backdrop with custom settings, the more work for the user if he wants to do a bulk change across all fields.

olafgrabienski commented 7 years ago

@docwilmot Yes, maybe I wasn't exactly understanding the meaning of "enabled". And yes, you and @jenlampton are right when you say that another overridden view mode may cause extra work. (I guess, often not very much work though.)

On the other hand, enabling/overriding the Full content mode by default, would be more transparent, which in a way causes less work. With Full mode enabled/overridden you don't have to think about which view mode to use when you see that there is a Full content mode which looks not enabled. Or wonder if it's better to use Full or Default. In my opinion, the full mode is a special case to this effect, because on many sites it's the most important one.

Actually, I don't want to argue on that question but share an aspect which had confused me already using Drupal. That said, I'm happy to have view modes in Backdrop core soon and to override Full mode by myself ;-)

jenlampton commented 7 years ago

another overridden view mode may cause extra work. (I guess, often not very much work though.)

It can be quite a lot if you have content with lots of fields - and since it's usually not necessary, it's extra work for no benefit (which is my real objection). These screenshots are from an average-ish Drupal (now Backdrop) site.

This is the most complex content type on the site: complex

This is the most basic: simple

With Full mode enabled/overridden you don't have to think about which view mode to use when you see that there is a Full content mode which looks not enabled. Or wonder if it's better to use Full or Default. In my opinion, the full mode is a special case to this effect, because on many sites it's the most important one.

Exactly, which is why I'm proposing that we don't reveal these other view modes to non-advanced users (hence the collapsed fieldset) to avoid the confusion we may be about to introduce. If all they see is Teaser and Default, as was the case before this change, that decision becomes much easier.

However, this brings up an interesting idea! What if we just made all other view modes inherit from Full content, and made Full content into THE default? (again, this isn't something we can do now, but it could be a good idea for better UX in Backdrop 2.x).

jenlampton commented 7 years ago

Back to terminology while we are thinking about other things...

Although I get why you used it, I'm not sure about the "Customize" as action. We seem to be using "Edit" elsewhere. Right?

We use "edit" for content and "configure" for configuration. In this case we are doing neither, we are activating something. We use "Enable" for this same action in Views, but as @docwilmot pointed out this is not the same thing, as these items are technically enabled even w/o specific display settings. That button also doesn't lead to an edit form, so I don't think "Edit" gives people an accurate idea of what the link/button would do...

If we can afford the horizontal space, then something more wordy like "Using the Default view mode", otherwise the "Using Default" seems OK.

I think we can do that, this column already has the appropriate mobile classes so it should disappear on smaller screens.

For the layouts, we use "Revert layout". In the Views list page we use plain "Revert". I think that if we need to economize, we can go with plain "Revert", but if we decide to use reset, then it'd have to be "Reset to Default".

The reason I went with Reset in the end is because everywhere we use Revert we are essentially rolling back to a previous state of the same thing. In this case, we are doing something more akin to a Disable so the word Revert didn't really feel right. I'm not married to Reset either, it was just the best I could come up with.

BTW, did we ever decide on "display modes" vs "view modes"?

I don't think we decided, but I moved that discussion to https://github.com/backdrop/backdrop-issues/issues/2376 since that can wait until after feature-freeze.

Also, I was thinking that perhaps instead of "Additional" view modes, we could say "Non-customized"? This would make it self-explanatory why these are inside this fieldset and which condition makes it so that view modes are moved to/from it.

I agree with the intent and will give it a try, but Non-customized feels a bit strange.

jenlampton commented 7 years ago

Here's a stab at some help text:

Content usually needs to be displayed differently as a Teaser in a list of other posts than when it is viewed on it's own page. Each way a piece of content can be displayed is called a View Mode.

The Default will be used for view modes that have not been explicitly customized. This is particularly useful when many view modes share the same field display settings. For example: Full content and Search index are usually the same, so both will often use the Default.

screen shot 2016-12-19 at 7 07 29 pm

docwilmot commented 7 years ago

This is too specific to Node entities, unless you plan unique descriptions for each entity type.

The PR also doesnt seem to address other entity displays; customization and resetting doesnt work elsewhere, gives "Page not found" on clicking either link.

Also:

MrHaroldA commented 7 years ago

I like where this is going!

If we're separating customized/non it doesnt seem necessary to have a column for status anymore

That seems true; but the 'Using the default view mode' is important information; and I like that label better than 'Non-customized view modes'.

klonos commented 7 years ago

Also, I was thinking that perhaps instead of "Additional" view modes, we could say "Non-customized"? This would make it self-explanatory why these are inside this fieldset and which condition makes it so that view modes are moved to/from it.

I agree with the intent and will give it a try, but Non-customized feels a bit strange.

...

If we're separating customized/non it doesnt seem necessary to have a column for status anymore

That seems true; but the 'Using the default view mode' is important information; and I like that label better than 'Non-customized view modes'.

All true. If we set the fieldset title to something descriptive, then the "Customize" would make much more sense. But "View modes using the default view mode" sounds bad and "View modes using the default settings" will make people wonder what the default settings are 😕

klonos commented 7 years ago

How about this for the help text:

When a piece of content is viewed on a page as part of a list of content of the same type, it may be required to be displayed in a different way than when it is viewed on its own page. Each way a piece of content can be displayed is called a View mode. So, while a piece of content can be viewed as full content, the same piece of content can also be displayed as a teaser or a summary for example. "Full content", "teaser" and "summary" are all view modes of the same piece of content.

...a bit wordy, but to the point I think + with examples to make it easier to understand.

docwilmot commented 7 years ago

See point above about being too Node specific. Remember this will also be on the display page for User and Taxonomy entities. As well as all fieldable contrib entities.

jenlampton commented 7 years ago

So, while a piece of content can be viewed as full content, the same piece of content can also be displayed as a teaser or a summary for example. "Full content", "teaser" and "summary" are all view modes of the same piece of content.

I'm not sure either of these last two sentences add any value that wasn't in the one previous. When it comes to help text, the more words we put on the screen, the less likely people are to read them. I think we can kill a lot of these words without loosing any value:

"When a piece of content is viewed on a page as part of a list of content of the same type, it is often required to be displayed in a different way differently than when it is viewed on its own page. Each way a piece of content can be displayed is called a View mode."

See point above about being too Node specific.

Though "content" works for both nodes and taxonomy terms, User accounts aren't really content. What if we used the word "items" instead?

When items are viewed as part of a list, they are often displayed in a different way than when viewed alone on a full page. Each way an item can be displayed is called a View mode.

MrHaroldA commented 7 years ago

Aren't there any entity labels available we could use instead of "item"?

jenlampton commented 7 years ago

We deliberately use "Content" instead of words like "Entity" to keep scary concepts out of the help text. I don't think we have any (good) examples of text like this in core that isn't specific to one entity type. We are running out of generic abstract nouns in the English language! :)

What's your objection to item?

MrHaroldA commented 7 years ago

Sorry, I should have said bundle labels.

When @bundle_label items are viewed as part of a list

jenlampton commented 7 years ago

ooh! good idea :)

jenlampton commented 7 years ago

Okay, I pushed an update to the PR that incorporates lots of recent suggestions: https://github.com/backdrop/backdrop/pull/1670

olafgrabienski commented 7 years ago

Oh, got a HTTP error 500 while adding a new view mode on the last PR.

jenlampton commented 7 years ago

whoops, committed some dpm()s. 1 sec. edit: Okay, sorry about that, try again now?