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

Adjust ImageForm logic to support new behaviour to be introduce in silverstripe/assets 1.6 #78

Merged
merged 1 commit into from
May 6, 2020

Conversation

maxime-rainville
Copy link
Contributor

@maxime-rainville maxime-rainville commented Mar 2, 2020

In Silverstripe CMS 4.6, the file modal will allow you to edit files directly. We're also altering the Image Placement form so it no longer displays all the image details, since you can now click on a button to view and edit the details.

This does lead to somewhat weird results when using the focus point module, because it wants to insert a FocusPointField on the image placement form. However, this field is useless because the focus point is control on the Image DataObject.

This PR addresses this problem, by being very specific when adding the FocusPointField. This is backward compatible as well. Existing installation will retain the same behaviour if they upgrade the focus-point module without upgrading to silverstripe-assets 1.6.

Related PR

Related issue

@maxime-rainville maxime-rainville force-pushed the pulls/master/ss16-support branch 2 times, most recently from e707d70 to f809a08 Compare March 2, 2020 03:39
- php: 7.1
env:
- DB=PGSQL
- php: 7.3
env:
- ASSET_VERSION=1.5.x-dev
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test against the old form layout

- ASSET_VERSION=1.5.x-dev
- php: 7.4
env:
- ASSET_VERSION=1.x-dev
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Test against the new layout

@maxime-rainville maxime-rainville force-pushed the pulls/master/ss16-support branch 4 times, most recently from bfa6577 to b7d24fd Compare March 2, 2020 04:40
@maxime-rainville maxime-rainville force-pushed the pulls/master/ss16-support branch 2 times, most recently from 1e72d2d to b2f1665 Compare March 2, 2020 05:09
@maxime-rainville maxime-rainville force-pushed the pulls/master/ss16-support branch from b2f1665 to a75e338 Compare March 2, 2020 05:19
@maxime-rainville
Copy link
Contributor Author

I've configured builds on the open-sausage repo. https://travis-ci.org/open-sausages/silverstripe-focuspoint/branches

The build is failing but it looks like the failure is pre-existing.

@jonom
Copy link
Owner

jonom commented Mar 2, 2020

Hey @maxime-rainville thanks so much for taking the time to preemptively patch this module. I will try to review in the next day or two.

@maxime-rainville
Copy link
Contributor Author

We haven't merged the matching asset-admin PR yet. We'll hopefully get that done in the next week or so.

This new feature won't be available until the 4.6 release, is about 2 months away. So it's not outrageously urgent.

@maxime-rainville
Copy link
Contributor Author

FYI I've referenced that people will need to update this module in the changelog silverstripe/silverstripe-framework#9425

@jonom
Copy link
Owner

jonom commented Apr 9, 2020

Hey @maxime-rainville sorry for the delay, has been a crazy month eh? I've just gone to test this change but since it relies on multiple PRs that haven't been merged yet I'm not sure the best place to start. Do you happen to have a composer.json configuration that would check out all the appropriate branches of the various pieces so I can test this?

Otherwise maybe you could bump me once the other PRs are merged when it will be easier for me to test?

@maxime-rainville
Copy link
Contributor Author

We haven't merged the other PRs yet. Probably best to wait until they are officially added to the codebase to review this one.

I'll see if we can get it added to our sprint. It just keeps getting push to the back burner.

Once the other PRs have been merged, give this composer file a go. Targeting "recipe-cms" 4.x-dev allows you to work off the unreleased code for the next minor release.

{
    "name": "test/project",
    "description": "Test project",
    "require": {
        "php": ">=7.1.0",
        "silverstripe/recipe-cms": "4.x-dev",
        "silverstripe/admin": "4.x-dev",
        "silverstripe-themes/simple": "~3.2.0",
        "jonom/focuspoint": "dev-pulls/master/ss16-support"
    },
    "require-dev": {
        "phpunit/phpunit": "^5.7"
    },
    "prefer-stable": true,
    "minimum-stability": "dev",
    "repositories": [
        {
            "type": "vcs",
            "url": "[email protected]:open-sausages/silverstripe-focuspoint.git"
        }
    ]
}

@maxime-rainville
Copy link
Contributor Author

Hi @jonom. The parent PRs finally got merged. The above composer.json file should now allow you to test it. Estimated release date for Silverstripe CMS 4.6 is late-June/early-July.

@jonom jonom merged commit 7b4806b into jonom:master May 6, 2020
@jonom
Copy link
Owner

jonom commented May 6, 2020

It works! Thanks @maxime-rainville.

@jonom
Copy link
Owner

jonom commented May 6, 2020

@maxime-rainville could I bug you for a little help with Travis? The last env you introduced (PHP 7.4 / ASSET_VERSION=1.x-dev) is failing with composer error. Would you mind taking a look and letting me know if there's an easy fix? https://travis-ci.org/github/jonom/silverstripe-focuspoint/builds/683967409

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.

2 participants