[CatalogPromotions] "For taxons" scope in UI with dynamic form configurations #13175
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Zales0123
force-pushed
the
ui-taxon-based-scope
branch
from
40a0db2
to
aafa357
Compare
GSadee
reviewed
Oct 7, 2021
There was a problem hiding this comment.
When we display the variants in the autocomplete, we display their name and code, but for the taxon only code, you could add the name there also to be consistent and more readable for administrators.
Before merge, could you check the taxon create/edit page in admin panel? Because during manual test, I came across strange behaviour that the tree with taxons disappeared. I've checked that also on master branch locally and it works. It looks like some problem with js on your branch, but I'm not sure 🤷🏻
src/Sylius/Bundle/CoreBundle/Resources/config/services/form.xml
Outdated
Show resolved
Hide resolved
Zales0123
force-pushed
the
ui-taxon-based-scope
branch
from
69cfeda
to
2502fe9
Compare
GSadee
approved these changes
Oct 8, 2021
| 'label' => 'sylius.ui.taxons', | ||
| 'multiple' => true, | ||
| 'required' => false, | ||
| 'choice_name' => 'code', |
There was a problem hiding this comment.
I would be for displaying taxons by their names or names + codes to be consistent with for variants scope and more readable for administrators
|
Thanks, Mateusz! 🎉 |
GSadee
added a commit
that referenced
this pull request
Oct 8, 2021
…Zales0123) This PR was merged into the 1.11-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | introduced in #13175 | License | MIT Fixing [this comment](#13175 (review)) Before: <img width="548" alt="Zrzut ekranu 2021-10-8 o 08 03 32" src="https://user-images.githubusercontent.com/6212718/136506249-1bd4cf8d-5c94-4902-8479-6809135f81a0.png"> After: <img width="512" alt="Zrzut ekranu 2021-10-8 o 08 02 29" src="https://user-images.githubusercontent.com/6212718/136506260-0a1318df-f833-47bd-9502-9ea4ac96a487.png"> Commits ------- 03b5fac [CatalogPromotions] Display taxons with names in scope
Zales0123
pushed a commit
to Sylius/SyliusCoreBundle
that referenced
this pull request
Dec 14, 2021
…Zales0123) This PR was merged into the 1.11-dev branch. Discussion ---------- | Q | A | --------------- | ----- | Branch? | master | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Related tickets | introduced in Sylius/Sylius#13175 | License | MIT Fixing [this comment](Sylius/Sylius#13175 (review)) Before: <img width="548" alt="Zrzut ekranu 2021-10-8 o 08 03 32" src="https://user-images.githubusercontent.com/6212718/136506249-1bd4cf8d-5c94-4902-8479-6809135f81a0.png"> After: <img width="512" alt="Zrzut ekranu 2021-10-8 o 08 02 29" src="https://user-images.githubusercontent.com/6212718/136506260-0a1318df-f833-47bd-9502-9ea4ac96a487.png"> Commits ------- 03b5fac46b25052d5dc68051e736f79685bdf6e9 [CatalogPromotions] Display taxons with names in scope
andrehoffmann30
added a commit
to punktDeForks/Sylius
that referenced
this pull request
Apr 19, 2022
coldic3
pushed a commit
to coldic3/Sylius
that referenced
this pull request
Nov 24, 2022
coldic3
pushed a commit
to coldic3/Sylius
that referenced
this pull request
Dec 5, 2022
coldic3
pushed a commit
to coldic3/Sylius
that referenced
this pull request
Dec 6, 2022
Rafikooo
added a commit
that referenced
this pull request
Dec 6, 2022
… and company name (andrehoffmann30, coldic3) This PR was merged into the 1.11 branch. Discussion ---------- | Q | A | |-----------------|--------------------------------------------------------------| | Branch? | 1.11 | | Bug fix? | yes | | New feature? | no | | BC breaks? | no | | Deprecations? | no | | Related tickets | fixes #13176 | | License | MIT | Commits ------- b50abea BUGFIX: #13176 add missing serialization configuration for phone number, company name 55b1b38 BUGFIX: #13175 move missing groups to defined attributes, remove duplication 97c70bc TASK: #13176 add unit test for address update on order 1370a9c TASK: #13176 use put api route to update order billing address 15fa6c5 BUGFIX: #13176 fix content type d92fdf0 TASK: #13176 adjust serialization config and test 6181dee BUGFIX #13176 fix test 463c5eb [PHPUnit] Fix OrdersTest::it_allows_to_patch_orders_address
coldic3
added a commit
that referenced
this pull request
Dec 7, 2022
* 1.11: [GitHub Actions] Restrict "Refactor" workflow to Sylius/Sylius repo Use MakerBundle instead of SensioGeneratorBundle [PHPUnit] Fix OrdersTest::it_allows_to_patch_orders_address BUGFIX #13176 fix test TASK: #13176 adjust serialization config and test BUGFIX: #13176 fix content type TASK: #13176 use put api route to update order billing address TASK: #13176 add unit test for address update on order BUGFIX: #13175 move missing groups to defined attributes, remove duplication BUGFIX: #13176 add missing serialization configuration for phone number, company name
coldic3
added a commit
that referenced
this pull request
Dec 7, 2022
* 1.12: [GitHub Actions] Restrict "Refactor" workflow to Sylius/Sylius repo Use MakerBundle instead of SensioGeneratorBundle [Psalm] Skip deprecated in Symfony 6.2 class and interface [PHPUnit] Fix OrdersTest::it_allows_to_patch_orders_address BUGFIX #13176 fix test TASK: #13176 adjust serialization config and test BUGFIX: #13176 fix content type TASK: #13176 use put api route to update order billing address TASK: #13176 add unit test for address update on order BUGFIX: #13175 move missing groups to defined attributes, remove duplication BUGFIX: #13176 add missing serialization configuration for phone number, company name
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As we already have the second Catalog Promotions scope type, it's time for UI... which is always a pain in the back, when we need to struggle with dynamic form configurations in collections 💃 I decided, that our traditional approach to the topic (used, for example) for cart promotions, has a too big technical overhead (with all registries, abstract configuration forms, prototypes), that I would like to try something new 🚀
Such an approach definitely needs some polishing (especially in javascript, cc @kulczy 😄), but should be fairly easy to use and extend. Note: for the first time, I've added form extension in AdminBundle, as the choices attributes are needed only for our default templates and are not application logic. I believe we should follow the path to fully decouple CoreBundle from the UI, especially if we want to be fully headless one day 🤯
After this PR and the other one with validation refactoring, adding new scope would be mostly adding service, form type and validator with proper tags, as well as adding a template in the
@SyliusAdmin/CatalogPromotion/Scopedirectory (could be parametrized, I suppose 🤔). It should definitely be included in the documentation 🏇 which will be provided in some of the next PRs🖖