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

API to define inline editing display #266

Closed
7 tasks done
brynwhyman opened this issue Jul 19, 2018 · 11 comments
Closed
7 tasks done

API to define inline editing display #266

brynwhyman opened this issue Jul 19, 2018 · 11 comments

Comments

@brynwhyman
Copy link

brynwhyman commented Jul 19, 2018

Blocks will be inline editable by default and will need to have an opt-out approach to in-line editing. Some blocks (eg. userforms) will not allow editing in-line as they are too complex.

This should probably be done as a private static declaration on block types, default is "in-line editable". Docs must be updated for block authors and some UI elements will be changed based on this setting.

A/Cs:

  • Opt-out configuration at block level for whether this block type is in-line editable
  • Docs updated to introduce this setting, and mention that it will not support GridField for now.
  • Blocks that are not marked as "inline-editable" will:
    • Not have an options action dropdown
    • Not have expand/collapse functionality
    • Have the expand chevron pointed to the right instead
    • Direct you to a GridFieldDetailForm when clicked

Options dropdown will be implemented in #322
Expand collapse functionality in #295 (probably)
GridDetailForm on click is already implemented

@robbieaverill
Copy link
Contributor

@chillu would be good to get your thoughts on this: if we're creating a new API like getOverviewFields() in order to determine the few form fields that are displayed when you initially load up inline editing of a block, where should the rest of the fields that getCMSFields() defines should be in Root.Main go?

If we add Content as a tab in the more actions dropdown (alongside Settings and History) then we'd be displaying the fields that are common between getCMSFields() and getOverviewFields() twice. If that's OK, then that's fine - what do you think?

The other question would be around us moving away from using GridField to power this (#273). Initially we won't have inline editing, so a CMS user would still click on an element in the editor view and be taken to an edit screen - I assume for simplicity that this would continue to be a GridFieldDetailForm? If that's the case, do we need to continue to use GridField as a backing for the ElementEditor, or can we still use parts of the GridField architecture without GridField being the entry point?

Another example would be the userforms block, which as per initial designs will not initially have inline edit capability even when inline editing is implemented. You'd set a message like "Click here to edit this element" or something similar, and be taken through to a GridFieldDetailForm to edit the data.

If we want to replace the GridFieldDetailForm implementation, perhaps we need to create an admin/blocks area which can be linked through to in order to edit content blocks...

Your thoughts would be great.

@chillu
Copy link
Member

chillu commented Jul 30, 2018

I assume for simplicity that this would continue to be a GridFieldDetailForm

I'd like to get to a point where there's one view to edit any record, and that view might be nested within different breadcrumb paths. Which means you'd need a generic API to get form fields for a particular type (as a form schema), rather than tying them to a GridFieldDetailForm and the presence of a GridField in the "containing" form one level up.

So you'd have admin/pages/edit/99 with a breadcrumb of Pages > About Us.
If you then click through to edit a block, you'd have admin/pages/edit/99/block/33, with a breadcrumb of Pages > About Us > Main Content Block. But the form schema would be loaded from /graphql?query={formSchema(type=TextBlock) ... without knowledge on where this edit form is embedded.

The tricky bit will be saving forms: Long-term, we want that to happen via mutations. But there's some unresolved complexities about the relationship between data validation, form fields and GraphQL fields. For now, maybe a generic end point? /graphql?mutation={saveForm(type=TextBlock) ... which uses a form provider in PHP?

This is the long-term perspective, let's figure out what's viable on the short-term without creating work that we need to throw away in a year.

If that's the case, do we need to continue to use GridField as a backing for the ElementEditor, or can we still use parts of the GridField architecture without GridField being the entry point?

I'd recommend making this UI driven by GraphQL APIs, rather than constrained by forms. Would be good to work through what implications this has for Open Sourcerers, we're happy to help.

@robbieaverill
Copy link
Contributor

Thanks for the feedback. @brynwhyman let's have a chat about this at some point soon

@robbieaverill
Copy link
Contributor

Note to self: write some RFCs to help us proceed with this

@chillu
Copy link
Member

chillu commented Aug 2, 2018

I'd like to get to a point where there's one view to edit any record,

This is now discussed in a separate card: silverstripe/silverstripe-admin#590

@ScopeyNZ
Copy link
Contributor

The current plan is to just show all fields if the block is "in-line editable". The different tab panes are controlled by options in the actions dropdown. So we just need to allow blocks to opt-in (or opt-out?) to being editable in-line. We can't have all blocks editable in-line - an example is "userform" blocks which are slated to be not in-line editable. The major blocker for that functionality right now is that we don't have a react alternative for GridFieldDetailForm (silverstripe/silverstripe-admin#590).

Currently setting up a form schema endpoint for elemental is trivial given we want all fields shown.

@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Aug 15, 2018

Updated the issue and removed the estimate for re-estimation.

@ScopeyNZ
Copy link
Contributor

Part 1 at #346

@robbieaverill robbieaverill self-assigned this Aug 27, 2018
@ScopeyNZ
Copy link
Contributor

@raissanorth I updated the last two A/Cs of this issue resolved by #353 - Can you confirm that PR addresses those points?

@raissanorth
Copy link
Contributor

Yes, #353 addresses the last to ACs.

@robbieaverill
Copy link
Contributor

PR at #356

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

No branches or pull requests

6 participants