NEW Edit tab click changes tab state #428
Conversation
raissanorth
changed the title
There was a problem hiding this comment.
The code looks good, although it could use a run over with a linter - there are a few extremely minor formatting issues around the place (to do with white space).
I can't help but think that this approach is a bad form of coupling though - e.g. There's no away an Element should be concerned with which tab is currently active - and even if it should be, to which Tabs component does it refer?
I feel like this is caught between a rock and a hard place in that the majority of controls we should like to affect are nested within the scaffolding component (FormBuilderLoader) - but as we discussed in person this PR is good to get a discussion going around the approach.
Perhaps there is a way to leverage the SilverStripe javascript Injector to wrap a higher order component around the Tabs component in this context, hooking it up to a redux reducer we control (as opposed to reduxform) in order to handle receiving the active tab value from an event dispatched directly to it (from wherever else that may be in hierarchy - where at the moment it's very coupled to the tree hierarchy presented in this PR).
I don't block this from a merge though, it can meet MVP and be followed up later in dealing with technical debt.
What are everyone else's thoughts?
| /** | ||
| * Set the active tab | ||
| * | ||
| * @param activeTab |
| @@ -48,7 +48,7 @@ class ElementList extends Component { | |||
| } | |||
|
|
|||
| return blocks.map((element) => ( | |||
| <div> | |||
| <div key={element.ID}> | |||
There was a problem hiding this comment.
I'm curious about whether giving it the same key as its child element might cause rendering issues... I don't know enough about how this works in React though
There was a problem hiding this comment.
In this case I think we can remove the key from the child component. I'm not 100% on this either but I'm pretty sure it's only used for the top level dom elements within a loop.
There was a problem hiding this comment.
Actually can we just remove that div? It doesn't seem to serve any purpose?
There was a problem hiding this comment.
"Adjacent JSX elements must be wrapped in an enclosing tag" 😉
|
|
||
| const sectionKey = 'DNADesign\\Elemental\\Controllers\\ElementalAreaController'; | ||
| const section = Config.getSection(sectionKey); | ||
| const formNameTemplate = section.form.elementForm.formNameTemplate; |
There was a problem hiding this comment.
You can use loadElementSchemaValue('formNameTemplate') for this (see client/src/state/editor/loadElementSchemaValue.js)
|
|
||
| /** | ||
| * Update the active tab on tab actions menu button click event. | ||
| * @param event onChangeEvent from a input field. |
There was a problem hiding this comment.
Missing prevProps declaration
raissanorth
force-pushed
the
pulls/4.0/tabby-the-cat
branch
2 times, most recently
from
646328f to
88a491a
Compare
raissanorth
changed the title
raissanorth
force-pushed
the
pulls/4.0/tabby-the-cat
branch
4 times, most recently
from
6d68844 to
09b2f63
Compare
| onClick={(event) => event.stopPropagation() | ||
| } | ||
| > | ||
| <ElementActionsComponent {...this.props} /></div> |
There was a problem hiding this comment.
What's this wrapping div doing? The </div> should be a on a new line.
There was a problem hiding this comment.
It allows to ignore the button press on the divider tab item.
| // {@see Tabs.getDefaultActiveKey} | ||
| const filterFieldsForTabs = (field) => field.component === 'Tabs'; | ||
| const defaultValue = state.form.formSchemas[elementFormSchema].schema.fields | ||
| .filter(filterFieldsForTabs)[0].children[0].name; |
There was a problem hiding this comment.
You could use find instead of filter here. Also, it'd probably be good to defensively program in for the possibility there's no tabs - although admittedly there's probably some work to do elsewhere for this...
| @@ -48,7 +48,7 @@ class ElementList extends Component { | |||
| } | |||
|
|
|||
| return blocks.map((element) => ( | |||
| <div> | |||
| <div key={element.ID}> | |||
There was a problem hiding this comment.
In this case I think we can remove the key from the child component. I'm not 100% on this either but I'm pretty sure it's only used for the top level dom elements within a loop.
raissanorth
force-pushed
the
pulls/4.0/tabby-the-cat
branch
from
09b2f63 to
552e989
Compare
raissanorth
force-pushed
the
pulls/4.0/tabby-the-cat
branch
from
552e989 to
5632e26
Compare
…her than storing it in local state.
raissanorth
force-pushed
the
pulls/4.0/tabby-the-cat
branch
from
5632e26 to
b9898ef
Compare
Related issue:
Related PRs:
This is work in progress for the above issue. The related PRs connect
Tabsto the redux-form store.I would like some initial feedback on the approach taken. Since we are dispatching an action I am not sure if bubbling up the edit tab click from
ElementActionstoElement, saving the active tab inElement's local state and passing it down as a prop toInlineEditFormis the most elegant solution.Outstanding work includes styling of the selected edit tab and cursor, as well as expanding the form on edit tab click.