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.

Sign up for GitHub

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

Jump to bottom

Move config from inline js to data attr #6964

Merged
merged 1 commit into from
Dec 30, 2022
Merged

Move config from inline js to data attr #6964

merged 1 commit into from
Dec 30, 2022

Conversation

vvasiloi
Copy link
Contributor

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

@vvasiloi vvasiloi force-pushed the feature/6354-admin-js-config-via-html-attr branch from 69eee45 to db33f66 Compare December 30, 2022 01:35
@alexander-schranz alexander-schranz changed the base branch from 2.5 to 2.6 December 30, 2022 01:45
@alexander-schranz alexander-schranz changed the title move config from inline js to data attr ove config from inline js to data attr Dec 30, 2022
@alexander-schranz
Copy link
Member

We will target the 2.6 branch as for this. Can you rebase it. The current PHP 8.1 test failing by doctrine can be ignore it is related to doctrine/dbal#5837

@vvasiloi
Copy link
Contributor Author

Oh, I used the wrong base branch...

@vvasiloi vvasiloi force-pushed the feature/6354-admin-js-config-via-html-attr branch from db33f66 to bd0f87b Compare December 30, 2022 01:57
@vvasiloi vvasiloi changed the title ove config from inline js to data attr Move config from inline js to data attr Dec 30, 2022
alexander-schranz
alexander-schranz requested changes Dec 30, 2022
View reviewed changes
Copy link
Member

@alexander-schranz alexander-schranz left a comment

Choose a reason for hiding this comment

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

Some small change request. Else looks good and the AdminControllerTest looks a lot better and easier understandable now. 👍

src/Sulu/Bundle/AdminBundle/Resources/js/index.js Show resolved Hide resolved
@alexander-schranz alexander-schranz added the Feature New functionality not yet included in Sulu label Dec 30, 2022
@vvasiloi vvasiloi force-pushed the feature/6354-admin-js-config-via-html-attr branch from bd0f87b to 70272e6 Compare December 30, 2022 02:11
@alexander-schranz alexander-schranz merged commit e9ab3de into sulu:2.6 Dec 30, 2022
@alexander-schranz
Copy link
Member

@vvasiloi Nice, thank you 👍

@alexander-schranz alexander-schranz mentioned this pull request Dec 30, 2022
Closed
@vvasiloi vvasiloi deleted the feature/6354-admin-js-config-via-html-attr branch December 30, 2022 13:07
@alexander-schranz alexander-schranz added this to the Release 2.6 milestone Feb 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-schranz alexander-schranz alexander-schranz approved these changes

Assignees
No one assigned
Labels
Feature New functionality not yet included in Sulu
Projects
None yet
Milestone
Release 2.6
Development

Successfully merging this pull request may close these issues.

Avoid inline Javascript in Admin main template
2 participants
@vvasiloi @alexander-schranz