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

WIP: sends table widget data on each ajax form submit #560

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

der-On
Copy link
Contributor

@der-On der-On commented May 18, 2022

Instead of sending data of a table widget only on a certain ajax handler (default onSave) the table widget now sends data on every ajax form submit.

What problem does this PR solve?

When having a translatable datatable widget data get's lost when switching locales.

In this example I show the current behaviour:

  1. We have the table in the "de" locale:

Bildschirmfoto vom 2024-11-06 10-31-11

  1. We switch to the "en" locale, an ajax request with the handler formProperties::onSwitchItemLocale is send, however the table will only handle the onSave handler by default:

Bildschirmfoto vom 2024-11-06 10-32-03

  1. We switch back to the "de" locale and all data is lost:

Bildschirmfoto vom 2024-11-06 10-32-30

How does this PR solve the problem?

By handling any ajax request handler the form data is preserved for each locale.

However this means datatables that are within forms that also allow other ajax handlers and must not handle those a specifique postbackHandlerName must now be added to the field config of the datatable to only listen to those ajax requests.

Instead of sending data of a table widget only on a certain ajax handler (default `onSave`) the table widget now sends data on every ajax form submit.
@der-On
Copy link
Contributor Author

der-On commented May 18, 2022

This PR yet does not contain the newly compiled JS files.

@LukeTowers LukeTowers added maintenance PRs that fix bugs, are translation changes or make only minor changes needs review Issues/PRs that require a review from a maintainer labels May 19, 2022
@LukeTowers LukeTowers added this to the v1.2.1 milestone May 19, 2022
@bennothommo bennothommo modified the milestones: v1.2.1, v1.2.2 Sep 12, 2022
@github-actions
Copy link

This pull request will be closed and archived in 3 days, as there has been no activity in this pull request for the last 6 months.
If you intend to continue working on this pull request, please respond within 3 days.
If this pull request is critical for your business, please reach out to us at [email protected].

@github-actions github-actions bot added the stale Issues/PRs that have had no activity and may be archived label Mar 14, 2023
@bennothommo bennothommo removed the stale Issues/PRs that have had no activity and may be archived label Mar 15, 2023
@der-On
Copy link
Contributor Author

der-On commented Mar 20, 2023

@LukeTowers Is this PR still "required" or desired to be merged?

@bennothommo
Copy link
Member

It is, but right now my priorities are on the marketplace. I will circle back to this when I have time.

@LukeTowers LukeTowers modified the milestones: v1.2.2, v1.2.3 Apr 20, 2023
@LukeTowers LukeTowers modified the milestones: v1.2.3, v1.2.4 Jul 7, 2023
@LukeTowers LukeTowers modified the milestones: v1.2.4, 1.2.5 Dec 27, 2023
@mjauvin mjauvin modified the milestones: 1.2.5, 1.2.6 Feb 18, 2024
@LukeTowers
Copy link
Member

@der-On @bennothommo what's the status of this PR?

@LukeTowers LukeTowers modified the milestones: 1.2.6, 1.3.0 Apr 25, 2024
@bennothommo
Copy link
Member

The PR works as described, however, it might be considered a BC break for the table widget behavior since now the table data is included with every AJAX request, not just the specified AJAX event.

I also think it will cause some issues for the Builder plugin, mainly on pages where there are other AJAX actions on the page besides the saving of data.

I'd be inclined to block this PR for the moment until we can ensure that the Builder plugin won't be affected.

@LukeTowers
Copy link
Member

@der-On are you interested in testing this change with the builder plugin so that we can move forward with merging it?

@der-On
Copy link
Contributor Author

der-On commented Apr 26, 2024

@LukeTowers I will try. Please assign it to me, so I do not loose it.

@der-On
Copy link
Contributor Author

der-On commented Jun 12, 2024

@bennothommo I've tried to test the builder, but that one seems broken currently.

Uncaught TypeError: can't access property "hide", $(...).data(...) is undefined
    makePluginActiveDone http://localhost/plugins/winter/builder/assets/js/build-min.js?v2.0.6:76
    jQuery 8
    Request http://localhost/modules/system/assets/js/framework.js?v=1.2.6:438
    request http://localhost/modules/system/assets/js/framework.js?v=1.2.6:505
    makePluginActive http://localhost/plugins/winter/builder/assets/js/build-min.js?v2.0.6:73
    applyPluginSettingsDone http://localhost/plugins/winter/builder/assets/js/build-min.js?v2.0.6:70
    cmdApplyPluginSettings http://localhost/plugins/winter/builder/assets/js/build-min.js?v2.0.6:65
    jQuery 8
    Request http://localhost/modules/system/assets/js/framework.js?v=1.2.6:438
    request http://localhost/modules/system/assets/js/framework.js?v=1.2.6:505
    cmdApplyPluginSettings http://localhost/plugins/winter/builder/assets/js/build-min.js?v2.0.6:64
    invokeCommand http://localhost/plugins/winter/builder/assets/js/build-min.js?v2.0.6:36
    triggerCommand http://localhost/plugins/winter/builder/assets/js/build-min.js?v2.0.6:493
    onCommand http://localhost/plugins/winter/builder/assets/js/build-min.js?v2.0.6:539
    jQuery 10
    <anonymous> http://localhost/modules/system/assets/js/framework.js?v=1.2.6:688
    <anonymous> http://localhost/modules/system/assets/js/framework.js?v=1.2.6:716
build-min.js:76:30

@bennothommo
Copy link
Member

@der-On what's the context? Which table?

@der-On
Copy link
Contributor Author

der-On commented Jun 13, 2024

@bennothommo I've opened the main page of the builder plugin, created a Test plugin using it and after that I get this error. I get the same error when clicking on the button to show me the existing plugins. At this point this PR was not merged.

@der-On
Copy link
Contributor Author

der-On commented Sep 3, 2024

I will retest it again with current dev versions as this is pretty urgent for us.

@der-On
Copy link
Contributor Author

der-On commented Sep 4, 2024

@bennothommo I get the same result with current dev-main version of the builder. The builder plugin seems to be partly broken in it's current state. I however was able to add tables and columns using the builder plugin with this PR applied.

@bennothommo

This comment was marked as outdated.

@bennothommo
Copy link
Member

@der-On have you tested this with a fresh install of Winter and the Builder plugin, with just your PR's changes applied?

@der-On
Copy link
Contributor Author

der-On commented Sep 5, 2024

@bennothommo Will try with fresh winter without translate plugin.

@LukeTowers
Copy link
Member

@der-On any updates on this?

@der-On
Copy link
Contributor Author

der-On commented Nov 6, 2024

@LukeTowers @bennothommo I've now found the time to test it against a fresh install of winter with dev-develop and builder plugin on dev-main. Builder seems to work but when creating model list definition I get the following error with my change:

this.cachedModelFieldsPromises[modelClass].done is not a function and endless recursion if hitting "add database columns".

Will investigate on this to find a workaround.

@der-On
Copy link
Contributor Author

der-On commented Nov 6, 2024

@bennothommo @LukeTowers I found a solution with a small fix to the PR and the builder. I've also edited the PR description to better describe the problem.

If no postbackHandlerName is set on a datatable it will update on each ajax request. However if a postbackHandlerName is defined on the table widget it will only update on that.

In the builder plugin I needed to add the following to plugins/winter/builder/classes/modellistmodel/fields.yaml:

tabs:
    stretch: true
    cssClass: master-area
    fields:
        columns:
            stretch: true
            cssClass: frameless
            tab: winter.builder::lang.list.tab_columns
            type: datatable
            postbackHandlerName: onModelListSave # <- this will ensure the table is only submitted on this handler

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance PRs that fix bugs, are translation changes or make only minor changes needs review Issues/PRs that require a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants