silverstripe / silverstripe-elemental

Create pages in Silverstripe CMS using content blocks
http://dna.co.nz
BSD 3-Clause "New" or "Revised" License
110 stars 115 forks source link

Add options for tabs within the more actions dropdown #323

Closed ScopeyNZ closed 6 years ago

ScopeyNZ commented 6 years ago

The designs indicate that the tab options that usually appear in a GridFieldDetailForm for blocks will need to be available in a dropdown on blocks. These need to be available per block without considerable performance degradation.

To handle this, I propose that we check tabs per block type (extension of BaseElement) and cache the result. There's is a possibility that the data saved on a block may affect the tabs that are shown, but when a block is expanded we can update the tab options dynamically (logged at #324).

A/Cs:

PR: #373

robbieaverill commented 6 years ago

For reference we worked on a similar component which never quite made it in, before the MoreActions React component was available in core: https://github.com/symbiote/silverstripe-gridfieldextensions/pull/225

brynwhyman commented 6 years ago

Think about generating a cache on dev/build / ?flush (implement Flushable)

chillu commented 6 years ago

@clarkepaul How important is this feature, compared to spending this time on other features, or having a slower CMS experience with a "simplistic" implementation? How likely do you think it is that authors navigate from the block overview into a specific tab via the "meatballs" menu, vs. just clicking "edit block" to get to the detail view, and then selecting the tab there? Has this shortcut been something out of user testing?

Update: Also, we'll be looking at caching all form fields for all record types in the CMS at some point, to speed up the overall authoring experience. This would also include tabs, and can be used for the "meatballs menu" as designed here. This would make any short-term solutions obsolete. Could we wait until we're in a position to do this properly?

ScopeyNZ commented 6 years ago

Hey @chillu. The designs currently do not indicate it is possible to get to the GridFieldDetailForm if the block is in-line editable. The only options are the tabs in this meatballs menu, the tabs rendered as "normal" - inside the in-line form, or as you say, providing the ability to access the edit (GridFieldDetailForm) screen and using the tabs that way.

chillu commented 6 years ago

I’m assuming that most content blocks will have a subset of form fields being inline editable (e.g. just title and content), and then all fields in the detail edit form? And those inline editable fields might contain fields across tabs (depending on priority), but there’s no tabs directly in the inline editing.

We have to create a solution which works for getCMSFields() implementations with 100+ fields, and 10+ tabs. And by "works", I mean set the expectation with devs that if they want blocks to be inline editable, they'll have to choose some fields as more important than others. Simpler getCMSFields() implementations might sometimes be fine to show with all fields and tabs inside the inline editing mode, but I think that's rare enough not to make it a design goal.

If we're loading the full getCMSFields here, I'm a bit concerned on what message that sends to devs (don't worry about choosing important fields), and what that does to CMS performance. We'd need to be smarter about deferred loading of form schema, and deferred rendering of form fields.

Apart from that, having a view mode toggle (tabs) hidden away in a "more options" dialog seems like a weird UX choice. After selecting a different view mode (different tab), you have no way of knowing you're in this mode without opening the menu/dialog again. And if we introduce a separate "current tab" viewer, it begs the question why we're not just using a GridField detail form here. Keep in mind that the eventual goal is for this detail edit form to load faster (caching form fields), so the mode switch won't be as painful for authors.

/cc @clarkepaul

brynwhyman commented 6 years ago

@chillu the current route being taken is for content blocks to be inline editable or not. Being built with an opt-out approach, any block that is set to be inline editable would by default display everything from the getCMSFields, see: #266. The assumption being that blocks with 100+ fields would have inline editing removed by a developer so to not adversely affect CMS performance.

Let's pause on this issue to make sure the performance impact is released and clarify the UX desire behind it.

clarkepaul commented 6 years ago

Lots going on in this conversation so I'll try to recap some of the reasoning.

How likely do you think it is that authors navigate from the block overview into a specific tab via the "meatballs" menu, vs. just clicking "edit block" to get to the detail view

I think the other tabs within the dropdown will be used far less than editing "content". Therefor easy access to the content tab was far more important. The whole idea is that people can edit the entire page without having to dive back and forth from blocks. Eventually we wont need access to the detail view for blocks unless specified by a dev (eg. userforms or large blocks). Having tabs in the dropdown wasn't vital at this stage but made for a much better experience letting people know where they are going and whats available to see. "Edit block" doesn't differentiate itself from inline editing very well. Some basic testing of the experience with settings and history in the dropdown worked well within the block view. It will also allow for tabs areas to be transitioned one by one if needed.

I’m assuming that most content blocks will have a subset of form fields being inline editable (e.g. just title and content), and then all fields in the detail edit form? And those inline editable fields might contain fields across tabs (depending on priority), but there’s no tabs directly in the inline editing?

Nope, the in-block editing view represents all the fields in the "Content" tab as being editable. Blocks naturally grow to the height of the fields inside. We are condensing groups of fields into single click areas to reduce consumed height (like a link which has: link text, link, title, new tab...). I'm trying to keep this as basic as possible (all fields treated the same). Tabs will transfer to blocks with the same structure (tab for tab), and nested tabs will continue to look like tabs with the in-block edit view.

I can't recall the exact details of the teams discussion on performance but pretty sure we had some options (hopefully captured somewhere), for example only loading the fields within a block when its expanded could be expected.

ScopeyNZ commented 6 years ago

Just to make it clear with a bullet pointed list of pertinent details:

Three options (in order of design preference I guess): A. Put tabs into the "more actions" menu B. Add an option to get to the edit form (and remove the in-line tabs) C. Keep the tabs in-line

We can't do the most preferable option without some sort of cache for the output of getCMSFields. FWIW I don't think this is very costly in regards to development time. If the idea is to have some sort of "CMS Fields Manifest" in the future then we will be writing this in a way that it can be replaced with low cost.

I'm still keen to do option A - I think the design benefit outweighs the technical debt, which we can minimise by containing it (And this is already estimated for it - only a 3).

NightJar commented 6 years ago

Sorry, what is the options menu we're talking about here? And specifically, how is it different from the EditForm(getCMSFields) for said Element?

ScopeyNZ commented 6 years ago

@NightJar I updated my comment to be more specific. The options menu always refers to the "More actions" menu aka. the meatballs.

I'm not sure what you're referring to with difference from EditForm. "More actions" isn't explicitly defined as part of getCMSFields or anything, but the designs have it as an amalgamation of the actions and form tabs of a block. Eg. Save, Publish but also Content, Settings, History.

NightJar commented 6 years ago

Yeah thanks for clarifying. I was getting confused with the use of Options and Settings occuring many times throughout the thread, and my thought process lead to Pages, where Settings is a tab, but a separate controller to getCMSFields (which is used in the form named EditForm). This pattern is often repeated in modules - but in that case it usually is a part of the normal tabset for getCMSFields - not a separate thing.

In which case putting the tabs into the more options menu and the settings tab would have been the same action point - so as confusion does it lead me to pose the question for clarification :)

chillu commented 6 years ago

Alright, thanks for taking the time to explain this - an important nuance that I didn't pick up on when reviewing the designs. But my fault for not being more involved in the discussion which lead to this point.

Essentially, we're forming developer habits here, one way or another. I'd rather push them towards not having tabs on inline editable tabs than creating workarounds for them. But devs sometimes don't control the presence of tabs (e.g. auto-added "link tracking" tabs), so this seems like a good compromise.

Would allow_inline_editing default to true?

There's is a possibility that the data saved on a block may affect the tabs that are shown, but when a block is expanded we can update the tab options dynamically

That would be my preference, since we don't need to come up with a short-term solution for getCMSFields caching. How hard is that? Even with the full "form field client-side cache" in place, that would only happen on demand - the first time the detail edit form (or inline edit form) is requested for this particular model. Which might be when you expand the block, not when you display its summary (TBC).

ScopeyNZ commented 6 years ago

I see the "(TBC)" but I'll address some of your statements already:

Would allow_inline_editing default to true?

TL;DR: Yes. We discussed this. The arguments for each side were "No, we want block developers to expressly allow in-line editing" and "Yes, most blocks will be in-line editable so that should be the default".

Which might be when you expand the block

This will be when you expand the block. In this case - we still want the short-term solution for caching tabs because we won't have any source of data until the block is expanded (but the tabs should appear in the "More Actions" menu from the get-go). Updating tab options dynamically when the form schema is loaded is something we should definitely do.

chillu commented 6 years ago

we still want the short-term solution for caching tabs because we won't have any source of data until the block is expanded (but the tabs should appear in the "More Actions" menu from the get-go)

So either you have to pre-load all cms fields for all blocks (before the first one could be expanded), which would be slow. Even if done async in the background, that's a lot of CPU cycles on the server. Or deal with a "more options" menu that doesn't have the tabs until you first expand each type - at which point you can implement client-side caching. My point is that I don't want to create a system which does the pre-loading without considering the bigger picture (GraphQL-based form schemas which are hopefully cached through Apollo). And if we wait until the first expand action, we might as well dynamically append those, and deal with caching properly later across all form fields incl. tabs.

ScopeyNZ commented 6 years ago

If we decide to not show tabs until the block is expanded, that would be amazing. If @clarkepaul is on board we don't really have a problem.

I agree that it's loads of CPU cycles on the server - hence the intention was to have a long-term server-side cache (that is cleared on flush). I agree that creating a system in elemental just for this is not ideal but does have some advantages over Apollo caching (as I see it):

Basically it's a minimum effort required to solve the problem with the fastest (perfomance-wise) solution that doesn't introduce hoards of technical debt. I'm keen to produce some code as a POC.

I've self-assigned this but I won't actually do anything till we've resolved these discussions.

clarkepaul commented 6 years ago

Are you suggesting that we have only actions in the dropdown when the block is collapsed, but if the block is expanded it has both the tab links and the block actions?

Its a bit hard for me to know the impact of the intended UI. If you find that development is blowing out because of showing the tabs in the dropdown menu, then we could hide the [...] icon until the block is expanded—as long as its the intention we continue to enhance the experience at a later date.

I think easy access to links is going to reduce clicking, especially when it comes to saving/publishing blocks.

ScopeyNZ commented 6 years ago

FYI @chillu - Here's the server side caching I implemented in the PR: https://github.com/dnadesign/silverstripe-elemental/blob/f1162762c6ef30a61b550fc823f3100e9b1a5b51/src/Services/ElementTabProvider.php

robbieaverill commented 6 years ago

MVP implemented in #373

I think this is good enough to close this ticket. The expensive operations are done on flush and cached, so there's negligible impact to the CMS user. @ScopeyNZ has also done a nice job of hiding away that part of the logic in an API marked as internal in case we come up with a better way to do it in future.