NEW Expand block preview #353
Conversation
raissanorth
force-pushed
the
pulls/master/expand-block-preview
branch
from
50f6d39 to
3bc5633
Compare
| fileUrl, | ||
| fileTitle, | ||
| content, | ||
| previewExpanded, |
There was a problem hiding this comment.
It feels like this should be a context argument
E.g. if (context === 'preview') { return <SummaryComponent ...props />; } else { return <FormBuilderComponent ...props />}
There was a problem hiding this comment.
Do you suggest I change the state on Element to, or only rename Content's prop and change the type to a string?
There was a problem hiding this comment.
I see - this is loosely coupled to the container's state name... leave it as is +D
| const { link } = this.props; | ||
| handleExpand() { | ||
| const { | ||
| element: { InlineEditable }, |
There was a problem hiding this comment.
maybe just import the element and use that` - the fact that the InlineEditable prop is capitalised is misleading, because usually capitalisation in React is used for component names, this sort of implies that it could be renderable
There was a problem hiding this comment.
This is coming from GraphQL, which is the reason it is capitalised. Would you still like me to rename it?
There was a problem hiding this comment.
Nope, but if you don't destructure it as much as this then you can refer to it as element.InlineEditable and it's fine =)
There was a problem hiding this comment.
Cool, I'll go for your last suggestion. But for consistency I'll do the same with ID, Title, and BlockSchema.
| @@ -66,11 +86,16 @@ class Element extends Component { | |||
| title={Title} | |||
| elementType={BlockSchema.type} | |||
| fontIcon={BlockSchema.iconClass} | |||
| link={link} | |||
| caretClickCallback={(event) => this.handleExpand(event)} | |||
There was a problem hiding this comment.
handleExpand is already bound to this in the constructor - I think you could use caretClickCallback={this.handleExpand} here
| @@ -39,6 +40,20 @@ class Header extends Component { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * Handle the opening/closing of the block preview | |||
| */ | |||
There was a problem hiding this comment.
Indentation slightly out
| 'btn', | ||
| { 'font-icon-right-open-big': !expandable, | ||
| 'font-icon-up-open-big': expandable && previewExpanded, | ||
| 'font-icon-down-open-big': expandable && !previewExpanded }); |
There was a problem hiding this comment.
Can you please format this object a little - each entry on a new line with consistent indentation
raissanorth
force-pushed
the
pulls/master/expand-block-preview
branch
from
3bc5633 to
dbf4b56
Compare
|
Note to self, rewrite the Behat tests! |
raissanorth
force-pushed
the
pulls/master/expand-block-preview
branch
from
dbf4b56 to
da5a68e
Compare
Resolves silverstripe/silverstripe-elemental-blocks#43
Block is not expandable:
Block is expandable and expanded; the preview is hidden
Block is expandable and not expanded; the preview is shown
Changes in this PR...
FormBuildercomponentSummarycomponentContentand rendered by itContentunit tests into theSummarytest file, since the latter is handling this logic nowContentcomponentElementcomponentDesign review by @clarkepaul still outstanding.
The expand button has been tested on Edge 17, IE 11 (browser stack), Chrome 11, Firefox 61, and Safari 11 . It is keyboard accessible on all of these browsers, expect Safari.
Also, the styling is slightly off on Edge 17: