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.

Sign up for GitHub

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

Jump to bottom

[Attributes] Add FloatAttributeType Feature #10690

Merged
merged 4 commits into from Mar 13, 2023
Merged

[Attributes] Add FloatAttributeType Feature #10690

merged 4 commits into from Mar 13, 2023

Conversation

panigrc
Copy link
Contributor

@panigrc panigrc commented Sep 22, 2019

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
License MIT

Selection_001

This feature adds a float attribute type. As future development it would be nice the scale option of NumberType to be configurable.

@panigrc panigrc requested a review from a team as a code owner September 22, 2019 10:25
@pamil pamil added the Feature New feature proposals. label Oct 4, 2019
pamil
pamil reviewed Oct 4, 2019
View reviewed changes
Copy link
Contributor

@pamil pamil left a comment

Choose a reason for hiding this comment

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

It looks good, however, we would need some tests for it, you can base them on our existing Behat scenarios:

features/product/managing_product_attributes/adding_integer_product_attribute.feature
features/product/managing_products/adding_product_with_integer_attribute.feature

Those are for integer attribute type, should be easy to create similar for float ones.

@panigrc
Copy link
Contributor Author

panigrc commented Oct 5, 2019

@pamil thank you 🙂. I will make the tests and will come back to you.

@Zales0123
Copy link
Member

@panigrc thank you for the scenarios, but it would be great to also have implementation for them :) 🖖 ☮️

@panigrc
Copy link
Contributor Author

panigrc commented Nov 13, 2019

@Zales0123 thank you for pointing that out, I saw that the Behat tests were failing.
I did a small change with the 1f39662 but I didn't find any other places in which I need to implement the scenarios.

I could use a little help to understand how to implement the scenarios.

@stale
Copy link

stale bot commented Feb 11, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Issues and PRs with no recent activity, about to be closed soon. label Feb 11, 2020
@Zales0123 Zales0123 added Do not stale Important issues and PRs, that should not be stalled by Stale Bot and removed Stale Issues and PRs with no recent activity, about to be closed soon. labels Feb 11, 2020
@vvasiloi
Copy link
Contributor

vvasiloi commented Jan 3, 2022

Hey @panigrc, do you still want to finish this? I can help with the tests.

@panigrc
Copy link
Contributor Author

panigrc commented Nov 17, 2022

Hi @vvasiloi, sorry for not answering sooner your message, I forgot about this PR.
I haven't used Sylius for a while now and I think it's better to reject this PR.

@vvasiloi
Copy link
Contributor

@panigrc no problem, maybe someone else will finish it.

@TheMilek
Copy link
Member

TheMilek commented Mar 9, 2023

Hi @panigrc!
Thank you for the PR 🍻
I like the feature, I'm going to try to finish it 🚀

@TheMilek TheMilek changed the base branch from 1.12 to 1.13 March 9, 2023 16:30
@panigrc
Copy link
Contributor Author

panigrc commented Mar 9, 2023

@TheMilek I'm grateful ! It's a long time ago since I pushed this PR.

@GSadee GSadee merged commit 27572d0 into Sylius:1.13 Mar 13, 2023
@GSadee
Copy link
Member

GSadee commented Mar 13, 2023

Thanks, Nik! 🥇

@panigrc panigrc deleted the correct-feature-new-float-attribute-type branch March 13, 2023 15:44
@TheMilek TheMilek mentioned this pull request Mar 20, 2023
Merged
GSadee added a commit that referenced this pull request Mar 21, 2023
@GSadee
bug #14867 [Attributes] FloatType input type as a number instead of t…
4ba988a 
…ext (TheMilek)

This PR was merged into the 1.13 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.13 <!-- see the comment below -->                  |
| Bug fix?        | yes                                                       |
| Related tickets | Related to  #10690                    
| License         | MIT                                                          |

<!--
 - Bug fixes must be submitted against the 1.12 branch
 - Features and deprecations must be submitted against the 1.13 branch
 - Make sure that the correct base branch is set

 To be sure you are not breaking any Backward Compatibilities, check the documentation:
 https://docs.sylius.com/en/latest/book/organization/backward-compatibility-promise.html
-->
By default the input type in NumberType is set to text, to change that to number We need to set html5 option to true, [ref](https://symfony.com/doc/current/reference/forms/types/number.html#html5)



Commits
-------

c80d39a [Attributes] Change FloatType input to number instead of text
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pamil pamil pamil left review comments

@GSadee GSadee GSadee approved these changes

Assignees

@TheMilek TheMilek

Labels
Do not stale Important issues and PRs, that should not be stalled by Stale Bot Feature New feature proposals.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@panigrc @Zales0123 @vvasiloi @TheMilek @GSadee @pamil