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

Fix automatic slug update on sbs form #2842

Merged
merged 1 commit into from
Aug 9, 2024

Conversation

lunars97
Copy link
Contributor

Short description

Currently the sbs form slug of the target language is not updated after the title change. It should be updated after the title of the page changes.

Proposed changes

  • Connect title input value with the slug input value so that the slug will change if the title changes

Side effects

  • should be none

Resolved issues

Fixes: #2797


Pull Request Review Guidelines

Copy link

codeclimate bot commented Jun 11, 2024

Code Climate has analyzed commit 4c34720 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (50% is the threshold).

This pull request will bring the total coverage in the repository to 83.3%.

View more on Code Climate.

@lunars97 lunars97 force-pushed the bugfix/automatic-slug-update-on-sbs-form branch 2 times, most recently from cfea3b7 to cd23bd9 Compare June 11, 2024 14:23
Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

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

Thank you for the PR 👍

Hmm, it would be cool if slugify_ajax() could be used here too instead of building another function 🤔 Having two different functions, we probably forget to update one of them if the logic changes in the future.

@theresantonie theresantonie requested review from theresantonie and removed request for theresantonie June 12, 2024 20:02
@theresantonie
Copy link
Contributor

@MizukiTemma wouldn't using the slugify_ajax() method mean that the slug is only updated when saving the form (backend)?
Implementing this as a client functionality seems more elegant to me.
It also looks to me as if slugifyStr and slugify_ajax() are different in their functionality.

@MizukiTemma
Copy link
Member

@theresantonie
slugify_ajax is callbed in the normal form by front-end events namely changes in the title field.
We can probably do the same for side-by-side view?

if (
    document.getElementById("id_title") &&
    (document.querySelector('[for="id_title"]') as HTMLElement)?.dataset?.slugifyUrl
) {
    document.getElementById("id_title").addEventListener("focusout", ({ target }) => {
        const currentTitle = (target as HTMLInputElement).value;
        const url = (document.querySelector('[for="id_title"]') as HTMLElement).dataset.slugifyUrl;
        slugify(url, { title: currentTitle }).then((response) => {
            /* on success write response to both slug field and permalink */
            slugField.value = response.unique_slug;
            updatePermalink(response.unique_slug);
        });
    });
}

@theresantonie
Copy link
Contributor

@MizukiTemma If this works the same, it is probably better to use an existing function. 👍

@lunars97
Copy link
Contributor Author

@theresantonie slugify_ajax is callbed in the normal form by front-end events namely changes in the title field. We can probably do the same for side-by-side view?

if (
    document.getElementById("id_title") &&
    (document.querySelector('[for="id_title"]') as HTMLElement)?.dataset?.slugifyUrl
) {
    document.getElementById("id_title").addEventListener("focusout", ({ target }) => {
        const currentTitle = (target as HTMLInputElement).value;
        const url = (document.querySelector('[for="id_title"]') as HTMLElement).dataset.slugifyUrl;
        slugify(url, { title: currentTitle }).then((response) => {
            /* on success write response to both slug field and permalink */
            slugField.value = response.unique_slug;
            updatePermalink(response.unique_slug);
        });
    });
}

slugify_ajax is used in the page form and it uses page as a model_type. Should it target page as a model_type? Because I am getting error NoReverseMatch and slugifyUrl returning undefined when I am using it. 🤔

@MizukiTemma
Copy link
Member

slugify_ajax is used in the page form and it uses page as a model_type. Should it target page as a model_type? Because I am getting error NoReverseMatch and slugifyUrl returning undefined when I am using it. 🤔

Yes, in page_sbs too :)

@lunars97 lunars97 force-pushed the bugfix/automatic-slug-update-on-sbs-form branch 2 times, most recently from a27c364 to 695c778 Compare June 20, 2024 10:05
Copy link
Member

@MizukiTemma MizukiTemma left a comment

Choose a reason for hiding this comment

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

Looks good 🎉 Thank you 😸

☝️ Only release note is left 😁

Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

@lunars97 could you please rebase this on develop? #2636 was recently merged and introduces changes and fixes both to the slugify_ajax method and the way it should be called from update-permalink.ts.

@lunars97 lunars97 force-pushed the bugfix/automatic-slug-update-on-sbs-form branch from ec880a6 to 5750540 Compare July 1, 2024 14:16
@lunars97 lunars97 requested a review from charludo July 2, 2024 08:42
@lunars97 lunars97 force-pushed the bugfix/automatic-slug-update-on-sbs-form branch from 60fa805 to d7f918f Compare July 9, 2024 18:07
Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! :)
Just one more thing

integreat_cms/static/src/js/forms/update-permalink.ts Outdated Show resolved Hide resolved
@lunars97 lunars97 force-pushed the bugfix/automatic-slug-update-on-sbs-form branch 2 times, most recently from f6ac47a to fa5b7c4 Compare July 22, 2024 14:10
@lunars97 lunars97 requested a review from charludo July 23, 2024 07:08
Copy link
Contributor

@charludo charludo left a comment

Choose a reason for hiding this comment

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

Super sorry for the lonf turnaround time with this review :(

This looks great now, thank you! 😄 🎉 I only have one nitpick left (see below), but will already approve this, since it's very minor.

integreat_cms/static/src/js/forms/update-permalink.ts Outdated Show resolved Hide resolved
@JoeyStk JoeyStk force-pushed the bugfix/automatic-slug-update-on-sbs-form branch from fa5b7c4 to 5b9ecbd Compare August 9, 2024 12:05
@JoeyStk JoeyStk force-pushed the bugfix/automatic-slug-update-on-sbs-form branch from 5b9ecbd to 4c34720 Compare August 9, 2024 12:11
@JoeyStk JoeyStk merged commit ed2f7a1 into develop Aug 9, 2024
5 checks passed
@JoeyStk JoeyStk deleted the bugfix/automatic-slug-update-on-sbs-form branch August 9, 2024 12:24
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.

Automatic slug update does not work on SBS form
5 participants