Enhancement: add injector to gallery toolbar #712

blueo · 2017-12-16T04:15:33Z

This is prototype for adding some injection points to the gallery section so the upload/create files buttons can be customised or added to.

Currently it is tricky to separate theses buttons because they effect changes that use the router/params. To this end, I have created an adminDecorator to allow passing of the router and location, along with common url-related functions to injected components (notably handleBrowse). This avoids needing to pass these objects down the component tree.

I'd love some feedback on my approach to using the injector and the decorator concept / better ways to setup injected components.

current plans include:

shift more state into redux to allow cleaner separation of components
inject the GalleryView component

relevant issues:

Refactor <Gallery> and <Editor> into containers and presentational components #179

blueo · 2018-01-08T02:21:38Z

happy new year @flamerohr :) I've removed the adminDecorator and changed GalleryToolbar so its buttons can be changed via props or new buttons can be added as 'children'. I've also moved the sorters into redux so they can be updated via the injectory (if you'd prefer this as a separate PR let me know :))

flamerohr

Overall, pretty neat work done, a few comments that would be good to get addressed :)

Let me know if you don't get time to apply the changes

flamerohr · 2018-01-15T00:13:40Z

client/src/components/GalleryToolbar/Buttons/AddFolderButton.js

+      onCreateFolder();
+    }
+    event.preventDefault();
+  };


you could have this handleCreateFolder defined as a const in the AddFolderButton function, did you prefer the factory paradigm instead?

I can see the benefit of simpler unit tests though, perhaps that could be added for this function as well :)

flamerohr · 2018-01-15T00:21:51Z

client/src/components/GalleryToolbar/Buttons/BackButton.js

+  handleMoveFiles,
+}) {
+  const { parentId: itemId } = folder;
+  if (itemId !== null) {


could we flip this and have the early return null instead

flamerohr · 2018-01-15T00:30:13Z

client/src/components/GalleryToolbar/Buttons/tests/BackButton-test.js

+
+  describe('render BackButton', () => {
+    it('should not render if parentId is not set', () => {
+      expect(Component({ ...props })).toBeNull();


I'd suggest importing React and writing this as if a react component <Component {...props} />
Makes it clear that we are primarily treating it that way.

huh so turns out renderIntoDocument doesn't play well with stateless components. (see https://stackoverflow.com/questions/36682241/testing-functional-components-with-renderintodocument). The options are to 1) leave it as it is or 2) add a wrapper. I'd prefer to leave it like this unless you already have a similar setup for other components?

huh...
I think we only have one or two stateless components and the rest as classes regardless of stateful-ness.
Could we convert the component is a class? (with eslint-disable if required)

I've been meaning to turn off the react/prefer-stateless-components rule for a while now.

flamerohr · 2018-01-15T02:18:40Z

client/src/components/GalleryToolbar/GalleryToolbar.js

+      // Button components
+      BackButton, // eslint-disable-line no-shadow
+      UploadButton, // eslint-disable-line no-shadow
+      AddFolderButton, // eslint-disable-line no-shadow


Could we give a different name to the imported components? BackButtonDefault or such?
I think the no-shadow linting rule is important to have

flamerohr · 2018-01-16T01:35:33Z

client/src/components/GalleryToolbar/tests/GalleryToolbar-test.js

+jest.mock('components/AssetDropzone/AssetDropzone');
+jest.mock('components/BulkActions/BulkActions');
+jest.mock('containers/MoveModal/MoveModal');
+jest.mock('../Buttons/BackButton');


I think we don't need to mock BackButton if you can override it in the props already?
Could mock all the child components in the props where possible

flamerohr · 2018-01-16T01:42:41Z

client/src/containers/Gallery/Gallery.js

+  // set default sort
+  if (!sort) {
+    sort = `${sorters[0].field},${sorters[0].direction}`;
+  }


how come this was moved from defaultProps?

The default sort was created from a constant but I've moved the sort info into redux so the default can't be created without the context from the store.

Sort wasn't stored in redux because it came from react-router url params - duplication of data.

Hmm... I didn't notice it being moved to redux.

sorry 'sort' does come from the router but I moved 'sorters' as otherwise it was impossible to customise the sort dropdown. But the result is that the default 'sort' needs the redux info from 'sorters'

oh, you're right!

neat, ok I can see why it's needed here now.

P.S. Perhaps have a truthy sorters[0] check, if it's customisable - someone's going to be naughty with it :D

flamerohr · 2018-01-16T01:43:36Z

client/src/containers/Gallery/Gallery.js


 export default compose(
+  inject(
+    ['GalleryToolbar'],
+    GalleryToolbar => ({ GalleryToolbar }),


this could be null for default behaviour :)

flamerohr · 2018-01-16T01:48:05Z

client/src/containers/TableView/TableView.js

-TableView.propTypes = galleryViewPropTypes;
+TableView.propTypes = {
+  ...galleryViewPropTypes,
+  sort: PropTypes.string.isRequired,


I see this belonging in the galleryViewPropTypes variable more than being standalone here and not required.

I'm pretty sure it was defined in a sharedPropTypes somewhere

It was in the shared props but I moved it for a couple of reasons:

Not all the components with shared props needed it (ThumbnailView).

I wanted to make the proptype check stricter on those components that did need it.

I could move it back to being an optional prop in the shared variable though if needed

ah, I overlooked ThumbnailView not needing it now (used to be, until the sorter got move up a level)

cool, this stays then :D

flamerohr

It all looks good to me, nice work.
One minor comment to address when you rebase

Can be merged on green

flamerohr · 2018-01-18T02:14:53Z

client/src/components/GalleryToolbar/Buttons/tests/AddFolderButton-test.js

+      const button = ReactTestUtils.renderIntoDocument(<Component {...props} />);
+      button.handleCreateFolder(new Event('click'));
+
+      expect(props.onCreateFolder).toBeCalledWith();


.toBeCalled() should be enough if no params are expected :)

created GalleryToolbar component GalleryToolbar Buttons components added as default props allowed adding extra buttons as children of GalleryToolbar

flamerohr · 2018-01-18T07:06:14Z

I had amended the props handleMoveFile and handleSort which I overlooked and should've been named onMoveFile and onSort :)

handleMoveFile and handleSort are valid class method names, but props should be on prefixed instead

flamerohr self-assigned this Dec 18, 2017

blueo force-pushed the pulls/add-injector-to-gallery-toolbar branch from 4e9f11b to b640840 Compare December 22, 2017 02:26

blueo force-pushed the pulls/add-injector-to-gallery-toolbar branch from b640840 to 4110b6e Compare January 8, 2018 02:11

blueo force-pushed the pulls/add-injector-to-gallery-toolbar branch 2 times, most recently from 560bd9a to c3702a1 Compare January 11, 2018 00:38

flamerohr added type/enhancement impact/high affects/v4 labels Jan 14, 2018

flamerohr added this to the Recipe 4.1.0 milestone Jan 14, 2018

flamerohr suggested changes Jan 16, 2018

View reviewed changes

flamerohr approved these changes Jan 18, 2018

View reviewed changes

Add injector to Gallery Toolbar

f81875a

created GalleryToolbar component GalleryToolbar Buttons components added as default props allowed adding extra buttons as children of GalleryToolbar

blueo force-pushed the pulls/add-injector-to-gallery-toolbar branch from c44238e to 8bd033f Compare January 18, 2018 02:55

blueo changed the title ~~[WIP] Enhancement: add injector to gallery toolbar~~ Enhancement: add injector to gallery toolbar Jan 18, 2018

Move sorters to gallery reducer to allow customisation

86b1188

flamerohr force-pushed the pulls/add-injector-to-gallery-toolbar branch from 8bd033f to 86b1188 Compare January 18, 2018 07:04

flamerohr merged commit 8d98c03 into silverstripe:1.0 Jan 18, 2018

flamerohr mentioned this pull request Mar 15, 2018

Add ability to use additional Toolbar buttons on the Gallery #751

Closed

unclecheese mentioned this pull request May 23, 2018

Add Refresh ability on the Gallery & Toolbar #784

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement: add injector to gallery toolbar #712

Enhancement: add injector to gallery toolbar #712

blueo commented Dec 16, 2017

blueo commented Jan 8, 2018 •

edited

Loading

flamerohr left a comment

flamerohr Jan 15, 2018

flamerohr Jan 15, 2018

flamerohr Jan 15, 2018

blueo Jan 17, 2018

flamerohr Jan 17, 2018

flamerohr Jan 15, 2018

flamerohr Jan 16, 2018

flamerohr Jan 16, 2018

blueo Jan 17, 2018

flamerohr Jan 17, 2018

blueo Jan 17, 2018

flamerohr Jan 17, 2018 •

edited

Loading

flamerohr Jan 16, 2018

flamerohr Jan 16, 2018

blueo Jan 17, 2018

flamerohr Jan 17, 2018

flamerohr left a comment

flamerohr Jan 18, 2018

flamerohr commented Jan 18, 2018

Enhancement: add injector to gallery toolbar #712

Enhancement: add injector to gallery toolbar #712

Conversation

blueo commented Dec 16, 2017

blueo commented Jan 8, 2018 • edited Loading

flamerohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flamerohr Jan 17, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flamerohr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

flamerohr commented Jan 18, 2018

blueo commented Jan 8, 2018 •

edited

Loading

flamerohr Jan 17, 2018 •

edited

Loading