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

Change to use injector for file upload fields in framework #6636

Merged

Conversation

flamerohr
Copy link
Contributor

@flamerohr flamerohr commented Feb 21, 2017

Created a FieldHandleField interface to use in place of UploadField (which had been pushed to a dependency), and use FileField by default in framework.

Requires:

@dhensby
Copy link
Contributor

dhensby commented Feb 21, 2017

The CMS behat tests are failing - I guess this is related?

@tractorcow
Copy link
Contributor

CMS tests need to be fixed separately. I'll come up with a patch.

What we want is an annotation on cms behat fixtures that disables certain tests when run with asset-admin installed, and then focus those tests (e.g. on upload field) for running in isolation.

@tractorcow
Copy link
Contributor

E.g.

@assets
@excludeIfModule asset-admin
Feature: Insert an image into a page
  As a cms author
  I want to insert an image into a page
  So that I can insert them into my content efficiently

@flamerohr flamerohr force-pushed the pulls/4.0/filehandlefield-class branch from 156dfdb to dac0ea1 Compare February 22, 2017 23:40
@tractorcow
Copy link
Contributor

If @sminnee can approve this, I've opted to remove this feature from the default insert link dialog. The issue is that react components don't work within any jquery-ui dialog due to the way that events are blocked by jquery UI (in a way that cannot be fixed without modifying jquery-ui directly).

The affected CMS tests are removed with silverstripe/silverstripe-cms#1740

@flamerohr flamerohr force-pushed the pulls/4.0/filehandlefield-class branch from 5a865b7 to a6c51f7 Compare February 23, 2017 04:14
@tractorcow tractorcow force-pushed the pulls/4.0/filehandlefield-class branch 3 times, most recently from 1f63977 to 84a1386 Compare February 24, 2017 01:39
@tractorcow
Copy link
Contributor

File media dialog is now removed; asset-admin module adds it back in. See silverstripe/silverstripe-asset-admin#387

Note that the CMS test removes the insert-media test fixtures, as these are now both implemented and tested via asset-admin.

@tractorcow
Copy link
Contributor

Added fix for behat silverstripe/silverstripe-behat-extension#145

* @returns Name/value array containing information about the plugin.
* @type Array
*/
getInfo() {
Copy link
Contributor Author

@flamerohr flamerohr Feb 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is unused and inaccessible, we could remove this

* @returns Name/value array containing information about the plugin.
* @type Array
*/
getInfo() {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is also unused and inaccessible, could remove it

@@ -45,7 +45,8 @@ const config = [
leaktools: `${PATHS.ADMIN_JS_SRC}/legacy/leaktools.js`,
MemberImportForm: `${PATHS.ADMIN_JS_SRC}/legacy/MemberImportForm.js`,
UploadField_select: `${PATHS.ADMIN_JS_SRC}/legacy/UploadField_select.js`,
TinyMCE_SSPlugin: `${PATHS.ADMIN_JS_SRC}/legacy/TinyMCE_SSPlugin.js`,
TinyMCE_ssmedia: `${PATHS.ADMIN_JS_SRC}/legacy/TinyMCE_ssmedia.js`,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could move this to the asset-admin repo instead right?

Christopher Joe and others added 5 commits February 27, 2017 10:38
@tractorcow tractorcow force-pushed the pulls/4.0/filehandlefield-class branch from 84a1386 to b3fc11e Compare February 26, 2017 21:58
@tractorcow
Copy link
Contributor

Feedback addressed; moved the plugin to asset-admin module.

@flamerohr
Copy link
Contributor Author

@tractorcow 's changes to my pull request look good, I think this is ready for merging

@tractorcow tractorcow merged commit eda4aae into silverstripe:master Feb 26, 2017
@tractorcow tractorcow deleted the pulls/4.0/filehandlefield-class branch February 26, 2017 23:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants