Skip to content
This repository has been archived by the owner on Apr 9, 2021. It is now read-only.

Category now has second description attribute that is displayed on the product list page above the product list #8

Merged
merged 5 commits into from Jul 31, 2018

Conversation

Miroslav-Stopka
Copy link
Contributor

@Miroslav-Stopka Miroslav-Stopka commented Jul 31, 2018

Q A
Description, reason for the PR Category: added second description attribute
New feature Yes
BC breaks No
Fixes issues #5
Standards and tests pass Yes/No (waiting for split)
Have you read and signed our License Agreement for contributions? Yes

@Miroslav-Stopka Miroslav-Stopka force-pushed the msa-category-second-description branch 2 times, most recently from a62840b to 983f9df Compare July 31, 2018 12:12
@Miroslav-Stopka Miroslav-Stopka changed the title WIP Msa category second description Category: added second description attribute Jul 31, 2018
@boris-brtan boris-brtan self-requested a review July 31, 2018 12:32
@boris-brtan boris-brtan self-assigned this Jul 31, 2018
@boris-brtan boris-brtan changed the title Category: added second description attribute Category now has second description attribute that is displayed on the product list page above the product list Jul 31, 2018
Copy link
Contributor

@boris-brtan boris-brtan left a comment

Choose a reason for hiding this comment

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

Great work, hope you enjoyed the enhancements.

@@ -0,0 +1,77 @@
<?php
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

automatically generated docblocks of PHPstorm are useless

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done :)

use Shopsys\FrameworkBundle\Form\Admin\Category\CategoryFormType;
use Shopsys\FrameworkBundle\Form\FormRenderingConfigurationExtension;
use Shopsys\FrameworkBundle\Form\GroupType;

Copy link
Contributor

Choose a reason for hiding this comment

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

hmm, wonder whether this blank line between use statements passes the standards

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, I have removed this empty line

@@ -37,3 +37,11 @@ services:
- { name: doctrine.event_subscriber, priority: -10 }

League\Flysystem\FilesystemInterface: '@main_filesystem'

Shopsys\FrameworkBundle\Model\Category\CategoryFactoryInterface: '@Shopsys\ShopBundle\Model\Category\CategoryFactory'
Copy link
Contributor

Choose a reason for hiding this comment

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

Please, consider to implement it the same way as Shopsys\FrameworkBundle\Model\Product\ProductFactoryInterface:
alias: Shopsys\ShopBundle\Model\Product\ProductFactory
since it is already merged into master

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Considered and applied

Shopsys\FrameworkBundle\Model\Product\Product: Shopsys\ShopBundle\Model\Product\Product
Shopsys\FrameworkBundle\Model\Category\Category: Shopsys\ShopBundle\Model\Category\Category
Shopsys\FrameworkBundle\Model\Category\CategoryDomain: Shopsys\ShopBundle\Model\Category\CategoryDomain
Copy link
Contributor

Choose a reason for hiding this comment

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

sorry but this has got problem with missing blank line at the end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Miroslav-Stopka Miroslav-Stopka force-pushed the msa-category-second-description branch from 983f9df to adaff04 Compare July 31, 2018 13:12
@Miroslav-Stopka
Copy link
Contributor Author

Hi @boris-brtan thank you for this review :) Please inform me about tests results.


### Changed
- [#1 - Basic changes in docs, readme etc. after copying from project-base](https://github.com/shopsys/demoshop/pull/1) : [@LukasHeinz]

### Fixed
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be no "Fixed" entry to changelog, I see entry in "Added" part and it is enough, isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to add a note to the fixed section because of this commit 03a30c0

@Miroslav-Stopka Miroslav-Stopka force-pushed the msa-category-second-description branch from 13a85fe to 03a30c0 Compare July 31, 2018 14:09
@Miroslav-Stopka Miroslav-Stopka merged commit 3b73fde into master Jul 31, 2018
@Miroslav-Stopka Miroslav-Stopka deleted the msa-category-second-description branch July 31, 2018 14:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants