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

[Attribute] Attribute types system modification #3608

Merged

Conversation

Zales0123
Copy link
Member

Q A
Bug fix? no
New feature? yes
BC breaks? yes
Deprecations? no
Fixed tickets
License MIT
Doc PR Sylius/Sylius-Docs#369

PR contains basic implementation of new attribute types system. It is partially inspired by Akeneo.
Each attribute type is now separate service, which is defined with type, storage type, form type etc. AttributeValue value is stored in database based on attribute type storageType.
For now, there are few simple attribute types implemented:

  • checkbox
  • date
  • datetime
  • integer
  • percent
  • textarea
  • text

Each of them has corresponding form type and it's name is set as 'sylius_attribute_type_'.$type.
Also creating attribute and adding attribute to product UI's have changed a little bit.

Attribute type is chosen as a first step of creation and it's not possible to change it later:
attribute-type-picker
attribute-create

While adding attribute to product, we first choose target attributes from modal. It is possible to pick more than one attribute at once. Added attribute forms are rendered with AJAX request.
attribute-picker
adding-attribute

It is really preliminary PR, so there is lots of work to do:

  • add options to attributes (e.g. min/max length to text, start/end date to datetime etc.)
  • add an awesome validation system, that will allows to validate easily each attribute type
  • implement some more default types (I would appreciate any ideas, as my creativity is unfortunately limited :))

@pjedrzejewski pjedrzejewski added this to the v0.17.0 milestone Nov 23, 2015
*/
public function getType()
{
return DefaultAttributeTypes::CHECKBOX;
Copy link
Member

Choose a reason for hiding this comment

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

I think these constants should be on *AttributeType classes
Storage type constants should live on AttributeValueInterface, because this is where we have storages defined. Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

So, here should be self::TYPE.

Copy link
Contributor

Choose a reason for hiding this comment

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

Or even static::TYPE so we can overwrite the contents of constant in child class.

Copy link
Member Author

Choose a reason for hiding this comment

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

@pjedrzejewski
Copy link
Member

Also, when we are at it - we should remove Presentation and make the name translatable.

Previously, name was serving as a unique code, now we have proper code property, so let's remove presentation and replace it with name. Make sure to keep it translatable. 👍

@pjedrzejewski
Copy link
Member

@Zales0123 And code should be unique, immutable after an attribute is created.

@aramalipoor
Copy link
Contributor

This is great job thanks @Zales0123, It would be awesome if we could have something like this for Options too.

It would be great if we could have a display type (alongside form and storage) which is the way we render the attribute/option in storefront e.g.:

  • Boolean type (Does it have bluetooth?) (✓)
  • Color palette type (T-Shirt's color)
  • Image type (Dress's pattern)

@pjedrzejewski
Copy link
Member

@aramalipoor For options - we will think about it. Actually, at some point I was thinking that Attributes could be used instead of Options in general - just like Akeneo does it, but it is complex and probably not for 1.0.

Regarding rendering - for attributes we plan to do it simple for now, where every attribute type will be able to have it's own template. Or even being able to select the template in backend from some list, defined ... somewhere, not sure where yet. :) Goal is to get this basic version merged (with validation and simple rendering) and then expand it even more - introduce scope (per channel), translatable values and unique attribute validation stuff.

@Zales0123
Copy link
Member Author

@pjedrzejewski Sure thing about presentation and code, I don't know why it's not implemented now, as it is quite obvious :)
@aramalipoor Thank you very much for your appreciation, I hope it's going to be easy and fun to play with attributes after this PR will be merged.

@aramalipoor
Copy link
Contributor

@pjedrzejewski That's awesome man! It's better to improve step by step ;)
@Zales0123 I'm waiting for this PR to implement it in our app ;)

@tristanbes
Copy link
Contributor

I send love to this PR ❤️ great job !

@Zales0123 Zales0123 force-pushed the attribute-types-system-modification branch 2 times, most recently from 70669f9 to 61ee0f8 Compare November 26, 2015 09:05
@Zales0123
Copy link
Member Author

@tristanbes Thank you for such a great optimism 😄 I really hope this change will reach expectations :)

@Zales0123 Zales0123 force-pushed the attribute-types-system-modification branch 2 times, most recently from 7234057 to 517136c Compare December 21, 2015 13:48
@Zales0123 Zales0123 force-pushed the attribute-types-system-modification branch from 517136c to b8bfa7c Compare December 22, 2015 07:14
@Zales0123
Copy link
Member Author

@pjedrzejewski @michalmarcinkowski @pamil @lchrusciel @stloyd @tuka217 @Arminek (plenty of people, isn't it?) Thank you all for the review :) I hope these changes that I've just pushed are final. I know there are some things to improve (some very good points from @pjedrzejewski) and I will be very happy to manage them on the base of this PR in the nearest future (at least I will try 😄)

pjedrzejewski pushed a commit that referenced this pull request Dec 29, 2015
…cation

[Attribute] Attribute types system modification
@pjedrzejewski pjedrzejewski merged commit 50cfdb5 into Sylius:master Dec 29, 2015
@pjedrzejewski
Copy link
Member

Boom! 💥 Thank you Mateusz! Great work. 👍

Anyone willing to add more configurations/validation or even whole new attribute types? Should be simple after this is merged. :)

@Zales0123 Zales0123 deleted the attribute-types-system-modification branch October 28, 2016 13:48
@Zales0123 Zales0123 restored the attribute-types-system-modification branch October 28, 2016 13:48
@Zales0123 Zales0123 deleted the attribute-types-system-modification branch October 28, 2016 13:48
pamil pushed a commit to pamil/Sylius that referenced this pull request May 7, 2019
…modification

[Attribute] Attribute types system modification
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

10 participants