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

Revert toolbar is not locked to the bottom of the screen when rendered inside a GridField detail form #22

Closed
robbieaverill opened this issue Jun 15, 2018 · 7 comments

Comments

@robbieaverill
Copy link
Contributor

robbieaverill commented Jun 15, 2018

image

CWP 2.1.0-rc1 with versioned-admin 1.0.x-dev and elemental 3.0.x-dev

Ideally the GridField actions are not shown when the History tab is opened (I think we've used a jQuery hide/show hack to this before) and the revert toolbar should be locked to the bottom of the screen.

This is visible when looking at a content block's history underneath a page.

Should look more like this (page level view):
image

@NightJar
Copy link
Contributor

NightJar commented Jun 28, 2018

I've looked through the logs for both Elemental and Versioned-Admin and this is not a regression as previously thought.
There is also a complication of the outer toolbar (that is locked to the base, with save/publish buttons) being drawn via entwine (jquery), and lives outside the domain of control of react, which only draws the inner panel contents.

A show/hide work around for the entwine toolbar is the only immediate solution, while locking the react component to the base of the inner panel. This would rely on react based components triggering entwine type events, which may cause some difficulty.

@NightJar
Copy link
Contributor

NightJar commented Jul 2, 2018

Looking into this it is caused by the manner in which the history interface is rendered into a normal (legacy style) content editing tab. I feel that proceeding to work around breaking out of the tab contextually would be adding quite a load of technical debt, as it requires the client app to contextually know about and deal with:

  • knowing which tabs don't want the toolbar rendered (save/publish)
  • extending the containing fieldset, tabset, tab, and inner content area to the base of the page (a load of CSS fiddling and specificity hacks in order to not break other areas of the CMS)
  • Introducing toolbar specific alignment workarounds in order to get things sitting as they do in the cms history viewer controller's layout - again more debt on a contextual basis for a limited use case.

Proposal:
Spend some time to implement a new GridFieldComponent that is essentially the history viewer. It can then render it's own main representation, without the impediment of another toolbar--south. The panel can be fully react rendered and essentially copy that of the Page style history view.

Downsides:
Would need to figure some way of triggering a URL change in the similar context of the current way the history is accessed. That means some way of mocking a Tab within a TabSet that is actually a link.
At the very worst this would be adding an empty Tab and an entwine click handler that redirects on click as opposed to simply switching DOM Node visibility (changing tabs).

How do people feel about this?
@brynwhyman @robbieaverill

@robbieaverill
Copy link
Contributor Author

robbieaverill commented Jul 2, 2018

In the context where HistoryViewerField is added to a FieldList in an automatically scaffolded CMS form, the problem is that the <fieldset> element doesn't continue the flexbox inheritance. Here are my suggestions:

  • Make the History tab load without AJAX
  • Overload LeftAndMain_EditForm.ss for this case (only for this case) and add flexbox-area-grow to the fieldset and necessary descendants so that the main history viewer body can use fill-height correctly

Alternatively we could implement the same thing with entwine Javascript hacks, commenting that they're there until the CMS is React driven.

This is basically a CSS fix, so let's try not to over complicate it too much if we can avoid it =)

@NightJar
Copy link
Contributor

NightJar commented Jul 2, 2018

This is a point in kind - the history tab currently doesn't load with AJAX (in relation to the other fields/tabs at least), all the fields are part of the edit form (which I think is the wrong place for history - but that's by the by), and are all loaded at once via the same request. This is that there are no separate endpoints to load specific tabs, to create one that is would essentially give us control over the entire form and we could return whatever we like. Granted there is the graphql endpoint that feeds in to populate the data of the HistoryViewerField, but that's about returning the data to populate it, rather than the initialisation of the field.

My major point here is that I think fiddling with workaround hacks in either CSS or Entwine is ultimately wasted effort and would be time consuming in a factor above what should be necessary.

@robbieaverill
Copy link
Contributor Author

robbieaverill commented Jul 2, 2018

My major point here is that I think fiddling with workaround hacks in either CSS or Entwine is ultimately wasted effort and would be time consuming in a factor above what should be necessary.

Sorry but I disagree. It's a necessary evil that we need to put up with until the CMS is React driven.

Yeah you're right re the XHR part, skip that:

  • Add fill-height to the History tab
  • When it's opened, add fill-height to the nearest parent fieldset and .tabset
  • When it's opened, hide the toolbar--south at the bottom of the page
  • Add height: calc(100% + 2*1.5385rem) to .elemental-block__history-tab .history-viewer__container
  • Add margin-left: -1.5385rem; margin-right: -1.5385rem; width: calc(100% + 2*1.5385rem) to the history viewer's .toolbar--south in elemental context

When History is navigated away from (any CMS tab apart from elemental-block__history-tab is clicked) remove the fill-height classes we added, but we can keep the rest.

Of course you'll want to implement this in a way that doesn't only apply to elemental, so add some new classes to identify it as being rendered in a GridField detail form.

@NightJar
Copy link
Contributor

NightJar commented Jul 3, 2018

Work around PR #27
silverstripe/silverstripe-elemental#251 required to vanquish exemplar issue.

@robbieaverill
Copy link
Contributor Author

Thanks for being patient on this @NightJar, all merged

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

3 participants