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

[framework] data object properties review #787

Merged
merged 21 commits into from Feb 13, 2019

Conversation

simara-svatopluk
Copy link
Contributor

Q A
Description, reason for the PR review of data object properties and documentation of usage
New feature No
BC breaks No
Fixes issues ...
Standards and tests pass Yes
Have you read and signed our License Agreement for contributions? Yes

@simara-svatopluk simara-svatopluk force-pushed the ss-data-objects-review branch 7 times, most recently from d46dc5b to 58b9bbe Compare February 4, 2019 13:28
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.

Hi,
looks good. I have just few questions and suggestions.

docs/introduction/entities.md Show resolved Hide resolved
docs/introduction/entities.md Outdated Show resolved Hide resolved
docs/introduction/entities.md Outdated Show resolved Hide resolved
@simara-svatopluk simara-svatopluk force-pushed the ss-data-objects-review branch 4 times, most recently from 462301c to ff9190e Compare February 6, 2019 14:06
@simara-svatopluk simara-svatopluk force-pushed the ss-data-objects-review branch 3 times, most recently from 7412b0e to aa477f2 Compare February 12, 2019 10:14
Svaťa Šimara added 14 commits February 12, 2019 12:27
… be always bool

- it is the way we use bool properties in data objects
- also the property is used everywhere as it is bool (not nullable)
- $name is localized, so it is an array of strings
- $sellingDenied is always boolean
…t value false becuase the code expects this variable to be bool
- now sets also $domainId because it is expected by the method name
…alue "1"

- because the value was already rewritten in all usages and now domainId properties are unified in all *Data objects
- it is the way we use bool properties in data objects
- also the property is used everywhere as it is bool (not nullable)
- it is the way we use bool properties in data objects
- also the property is used everywhere as it is bool (not nullable)
Svaťa Šimara added 2 commits February 12, 2019 12:27
- for all locales
- initializes with null (as the current admin form does)
- changed phpdoc of multilanguage properties appropriately
- fixed code snippets in documentation
Copy link
Contributor

@PetrHeinz PetrHeinz left a comment

Choose a reason for hiding this comment

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

I have a couple of suggestions for the docs:

docs/introduction/entities.md Outdated Show resolved Hide resolved
docs/introduction/entities.md Outdated Show resolved Hide resolved
docs/introduction/entities.md Outdated Show resolved Hide resolved
@PetrHeinz
Copy link
Contributor

Localization::getLocalesOfAllDomains() should use the new method Domain::getAllLocales() intenally, otherwise it's a DRY violation

Svaťa Šimara added 2 commits February 13, 2019 07:53
…llLocales(), to avoid code duplication

- changed return value of Domain:getAllLocales() to keyed array so it matches Localization method perfectly
@simara-svatopluk simara-svatopluk merged commit 04155aa into master Feb 13, 2019
@simara-svatopluk simara-svatopluk deleted the ss-data-objects-review branch February 13, 2019 10:07
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.

None yet

3 participants