jonom / silverstripe-focuspoint

Module for improving automatic image cropping in SilverStripe. Adds simple art-direction control by allowing you to set and crop from a focus point instead of the centre point of an image.
MIT License
109 stars 47 forks source link

Focus point UI persistance #34

Closed digitall-it closed 6 years ago

digitall-it commented 7 years ago

The focus point gets saved, but the UI resets to the center of the picture on load each time. It doesn't load back the focus point. That probably means that you if you want to change other image properties like the title, you have to set again the focus point or it will be saved in the center.

Tested on OSX Safari.

jonom commented 7 years ago

Hi @digitall-it, the saved focus point loads fine for me in Safari. Maybe you have a conflict with another module? Can you open up your browser console while the file interface is loading and see if there are any errors?

digitall-it commented 7 years ago

Glad to know it should work, your module is awesome. Thank you for making it. As the problem seems to be installation-wise, allow me some time to fully investigate the issue.

digitall-it commented 7 years ago

I used the module in another installation, does the same. It saved the focus point, but if I navigate away from the image, then return to it, the cross is in the center again. If you change other details like the title of the image, as the cross is in the center, the focus point resets to the center, you have to move it again in the new position. No js errors. Does the same without kickassets module. Tried with two browsers on OSX and Windows. Would you like for me to test in a minimum use case?

Here is my composer.json

{
    "name": "silverstripe/installer",
    "description": "The SilverStripe Framework Installer",
    "require": {
        "php": ">=5.3.3",
        "silverstripe/cms": "3.4.1",
        "silverstripe/framework": "3.4.1",
        "silverstripe/reports": "3.4.1",
        "silverstripe/siteconfig": "3.4.1",
        "silverstripe-themes/simple": "3.1.*",
        "burnbright/silverstripe-bootstrap": "^1.0",
        "dnadesign/silverstripe-elemental": "dev-master",
        "bluehousegroup/silverstripe-data-object-version-viewer": "dev-master",
        "silverstripe/userforms": "^4.1",
        "colymba/gridfield-bulk-editing-tools": "^2.1",
        "silverstripe/secureassets": "^1.1",
        "burnbright/silverstripe-bootstrap-shop": "dev-master",
        "tractorcow/silverstripe-opengraph": "^3.1",
        "silverstripe/googlesitemaps": "^1.5",
        "jonom/silverstripe-share-care": "^1.2",
        "jonom/focuspoint": "^2.2",
        "bummzack/sortablefile": "^1.2",
        "silverstripe/silverstripe-omnipay": "^2.0",
        "bummzack/silverstripe-omnipay-ui": "^0.1.1",
        "omnipay/paypal": "^2.6",
        "omnipay/stripe": "^2.4",
        "omnipay/manual": "^2.2",
        "thisisbd/silverstripe-fontawesome-iconpickerfield": "*",
        "thisisbd/silverstripe-fixjpeg-orientation": "^0.1.0",
        "silverstripe/kickassets": "3.2.3"
    },
    "require-dev": {
        "phpunit/PHPUnit": "~3.7@stable",
        "oddnoc/silverstripe-artefactcleaner": "^3.0"
    },
    "config": {
        "process-timeout": 600
    },
    "prefer-stable": true,
    "minimum-stability": "dev"
}
jonom commented 7 years ago

Yes please can you test on a fresh install with only FocusPoint add-on. I think you should find it works in that case, then maybe try adding these modules back one at a time and see if they introduce the issue:

jonom commented 7 years ago

@Martindoorneakamp let's continue discussion here as you seem to be affected by the same issue.

digitall-it commented 7 years ago

In my case, the focus point is correctly saved and the cropping functions work as expected. Just the UI doesn't show the changes.

@Martindoorneakamp could you check out that?

Martindoorneakamp commented 7 years ago

In my case, the does not work, and the on the front end the focus does not get applied. But it seems like everything is getting loaded properly, And as said before an exact clone of the site is working as intended on my local mamp setup.

jonom commented 7 years ago

@Martindoorneakamp you said though that if you make a change, it does get saved in the DB right? I am working on this right now, I think the problem is that the image is not available initially which is required to position the grid correctly. The fallback .load() method doesn't appear to be firing (image.load is known for being unreliable). I'm looking to solve it by working out all the dimensions server side so we don't need to wait for the image to render before positioning the grid.

Martindoorneakamp commented 7 years ago

Thats the bizarre thing, it saves properly to the db and works locally, and in older versions of silverstripe. The version of silverstripe i',m experiencing the problems is 3.5.0 btw.

jonom commented 7 years ago

Can you guys please test this fix and get back to me: https://github.com/jonom/silverstripe-focuspoint/tree/fix-image-load

Martindoorneakamp commented 7 years ago

Thank for trying Jonom, but sadly no changes after applying your fix.

Martindoorneakamp commented 7 years ago

I just noticed something weird, but i'm not sure if this is causing the problem: Local the $FocusX $FocusY is outputting (Two diferrent pictures/settings): -0.76888888888889 -0.84 Live: 0,0533333333333 -0,53125

But as you can see localy the values are printed with a ".", and Live ",".

Martindoorneakamp commented 7 years ago

I found out what was going wrong, my set_locale('nl_NL'); was influencing the notation off the upper fields, as shown above.

jonom commented 7 years ago

Ahhhhhhh. Okay, so the solution could be to replace a comma if found with a period when reading the value in to the script?

jonom commented 7 years ago

@digitall-it are you also using a locale that uses comma instead of period?

BTW I have added some debugging docs to that new branch that may be handy.

Martindoorneakamp commented 7 years ago

Yes, i think that could work!

jonom commented 7 years ago

Have pushed up a new fix to that branch, want to try it out? I'm still entering new numbers in the fields with period instead of comma but it sounds from what you're saying like that should work fine?

digitall-it commented 7 years ago

Yes I use a locale that uses comma instead of period in the decimal notation!

jonom commented 7 years ago

Looks like the mystery is solved. If one or both of you could try out that new fix and confirm it works for you, I'll do a release.

Martindoorneakamp commented 7 years ago

I will test, and get back to you tommorow. Thanks!

digitall-it commented 7 years ago

My tests are inconsistent: at the moment, the plugin seems to work correctly without applying the patch, so I rely too on @Martindoorneakamp's findings.

Martindoorneakamp commented 7 years ago

I tried the update, and it does not work sadly. I have removed the locale settings for now to ensure that focuspoint works. The $FocusX and $FocusY where still printed with a ,

jonom commented 7 years ago

@Martindoorneakamp can you post the yml you're using to set the locale? I want to try changing the locale myself, the fix I posted was just theoretical

jonom commented 7 years ago

I tried setting the overall locale to Finnish as well as changing the locale to Finish on my member profile but I couldn't get the fields to show a value with a comma. I can't reproduce the problem so will need some steps to reproduce from you @Martindoorneakamp before I can look at it any further.