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

Add Router Attributes to Form Request to ResourceTabs View #6985

Draft
wants to merge 1 commit into
base: 2.5
Choose a base branch
from

Conversation

alexander-schranz
Copy link
Member

@alexander-schranz alexander-schranz commented Jan 30, 2023

Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Fixed tickets fixes #
Related issues/PRs #
License MIT
Documentation PR sulu/sulu-docs#

What's in this PR?

Add Router Attributes to Form Request to ResourceTabs View

Why?

Currently the ResourceTabView is missing some important feature to set this kind of options.

TODO

  • Add addRouterAttributesToFormRequest to createResourceTabViewBuilder
  • Avoid duplicated logic

@alexander-schranz alexander-schranz added the Bug Error or unexpected behavior of already existing functionality label Jan 30, 2023
@alexander-schranz alexander-schranz force-pushed the bugfix/resource-tabs-router-attributes-to-form-request branch from d66015b to bde6b84 Compare January 30, 2023 11:40
@ThomasStoevelaar
Copy link

With this change we see 3 GET requests when navigation to a Form view inside a ResourceTabs view like the edit form of a category. This doesn't happen when refreshing the page when you are already on the form.

If you are adding missing features to the ResourceTabs I think it's also a good idea to add setIdQueryParameter to the createResourceTabViewBuilder and pass the idQueryParameter to the ResourceStore in the ResourceTabs view component if set.

createResourceStore = () => {
    const options = {};
    if (this.locales) {
        options.locale = observable.box();
        this.router.bind('locale', options.locale);
    }

    if (this.resourceStore) {
        this.resourceStore.destroy();
    }

    const {
        router: {
            attributes,
            route: {
                options: {
                    idQueryParameter,
                    requestParameters = {},
                    routerAttributesToFormRequest = {},
                },
            },
        },
    } = this.props;

    const formStoreOptions = requestParameters ? requestParameters : {};

    Object.keys(toJS(routerAttributesToFormRequest)).forEach((key) => {
        const formOptionKey = routerAttributesToFormRequest[key];
        const attributeName = isNaN(key) ? key : routerAttributesToFormRequest[key];

        formStoreOptions[formOptionKey] = attributes[attributeName];
    });

    if (idQueryParameter) {
        this.resourceStore = new ResourceStore(this.resourceKey, this.id, options, formStoreOptions, idQueryParameter);
    } else {
        this.resourceStore = new ResourceStore(this.resourceKey, this.id, options, formStoreOptions);
    }
};

@alexander-schranz alexander-schranz added the To Discuss The core team has to decide if this will be implemented label Feb 14, 2023
@alexander-schranz
Copy link
Member Author

@sulu/core-team we need to discuss how we can avoid duplicated code here. The ResourceTabs.js is now reimplementing a lot logic we already implemented for the FormView.

@alexander-schranz alexander-schranz force-pushed the bugfix/resource-tabs-router-attributes-to-form-request branch from bde6b84 to 3f38f75 Compare February 20, 2023 15:38
@alexander-schranz alexander-schranz added To Discuss The core team has to decide if this will be implemented and removed To Discuss The core team has to decide if this will be implemented labels Feb 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Error or unexpected behavior of already existing functionality To Discuss The core team has to decide if this will be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants