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

Update login.html.twig #1430

Merged
merged 8 commits into from
Dec 6, 2021
Merged

Conversation

AntoineRoue
Copy link
Contributor

@AntoineRoue AntoineRoue commented Nov 4, 2021

Subject

Taking into account the following change : "title_mode" (with values "single_image", "single_text", "both") is deprecated since sonata-project/admin-bundle 3.104 and replaced by "logo_content" (with values "icon", "text", "all").

I'm not sure if I should've added a comment "{# NEXT_MAJOR: Remove the title_mode check #}" because it is for the next major of SonataAdminBundle.

Changelog

### Fixed
- Taking into account sonata_admin.options.logo_content configuration value

Taking into account the following change : "title_mode" (with values "single_image", "single_text", "both") is deprecated since sonata-project/admin-bundle 3.104 and replaced by "logo_content" (with values "icon", "text", "all").
Using the exact same conditions as in standard_layout.html.twig file of SonataAdminBundle.
@jordisala1991
Copy link
Member

You should also update minimum version of the admin bundle, because previous versions do not have this code deprecated

@AntoineRoue
Copy link
Contributor Author

AntoineRoue commented Nov 5, 2021

You mean setting "sonata-project/admin-bundle": "^3.104", in composer.json, instead of ^3.76 currently ? If so, I'm not sure we can do this easily.

@jordisala1991
Copy link
Member

Yes, And we are able to do this, its not a bc break to update a dependency, you just have to change the version on the composer.json

@VincentLanglet VincentLanglet requested a review from a team November 28, 2021 20:23
@jordisala1991
Copy link
Member

I started to work on 5.x branch to prepare for SF 5 / 6, without FOSUser and all. Please take a look at how I solve this on tihs precise PR to copy the same code: #1444

This will help with the merge to higher branches.

@AntoineRoue
Copy link
Contributor Author

Do you want me to update checkEmail, request and reset files as well in this PR ?

Compared to my changes, you removed the conditions and ('single_text' == sonata_config.getOption('title_mode') or 'both' == sonata_config.getOption('title_mode')), isn't it ? However, as the option title_mode is deprecated but not yet removed, how can you not take title_mode value into account ?
If title_mode configuration is changed to single_text or single_image, with your change it will always display both logo and text.

@jordisala1991
Copy link
Member

jordisala1991 commented Dec 6, 2021

You're right, my PR is only for Sonata 4, where I do not take into account the old deprecated values, because they are removed on Sonata 4. And yes, it would be nice to fix those 3 templates at once, it should share the same fix.

@AntoineRoue
Copy link
Contributor Author

Finished my copy paste.

@VincentLanglet
Copy link
Member

@AntoineRoue Can you add a changelog in your first message to explain the purpose of this PR. This will be used for the next release. You can take a look at other PR to see the format.

@AntoineRoue
Copy link
Contributor Author

Done. Is that good ?

@VincentLanglet VincentLanglet merged commit c83e69d into sonata-project:4.x Dec 6, 2021
@VincentLanglet
Copy link
Member

Done. Is that good ?

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants