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

#4068: Provide number widget #4069

Closed
wants to merge 6 commits into from

Conversation

stefan-korn
Copy link
Contributor

fixes #4068

  • Test coverage exists
  • Documentation exists

QA Steps

  • Add manual QA steps in checklist format for a reviewer to perform to confirm that the feature or fix is working. Include as much details as possible so that the reviewer doesn't lose time figuring out how to perform steps.

@janette
Copy link
Member

janette commented Dec 4, 2023

@stefan-korn this would need test coverage before it can be merged.

@dafeder
Copy link
Member

dafeder commented Dec 11, 2023

@stefan-korn are you able to add some coverage? The easiest thing would probably be to add a number field to Drupal\Tests\json_form_widget\Unit\JsonFormBuilderTest

Copy link
Member

@dafeder dafeder left a comment

Choose a reason for hiding this comment

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

I think this will satisfy codeclimate about code/doc standards

Comment on lines 417 to 422
* @param mixed $spec
* Object with spec for UI options.
* optional specs
* - step
* - min
* - max
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param mixed $spec
* Object with spec for UI options.
* optional specs
* - step
* - min
* - max
* @param object $spec
* Specification for UI options. Optional properties:
* - step: Ensures that the number is an even multiple of step.
* - min: Minimum value.
* - max: Maximum value.

Comment on lines 426 to 428
* @return array
* The element configured as number.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @return array
* The element configured as number.
*/
* @return array
* The element configured as number.
*
* @see \Drupal\Core\Render\Element\Number
*/

@dafeder dafeder self-assigned this Apr 5, 2024
Copy link
Member

@dafeder dafeder left a comment

Choose a reason for hiding this comment

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

@stefan-korn have given this another review. See my one comment here, I was able to cause an error by giving the ui schema a step but not a max.

I'd like to get this in, but I think it needs a little more work. Beyond the comments in this and previous reviews, I think it should also work to define a field as "number" directly in the schema, with no additional definition in the UI schema. This may mean adding a new "helper" class, or else consolidating "number" and "integer" since they ultimately use the same field type.

if (isset($spec->min)) {
$element['#min'] = $spec->min;
}
if (isset($spec->step)) {
Copy link
Member

Choose a reason for hiding this comment

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

@stefan-korn shouldn't this be $spec->max?

@stefan-korn
Copy link
Contributor Author

@dafeder : I addressed your comments.

Regarding "number" in the schema. I wasn't aware that there is the integer field. It is only used once in the data-dictionary.json I suppose and we do not need data dictionary right now, so I missed that.

Should the schema be configurable about step, min and max? That way no widget "number" would be needed at all?

@dafeder
Copy link
Member

dafeder commented Jul 3, 2024

@stefan-korn re-thinking this I think the "number" field in the schema itself can be dealt with separately, should not be a blocker for this. I did a new PR from this that hits the test coverage, closing this in favor of #4215. Thanks!

@dafeder dafeder closed this Jul 3, 2024
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.

Number widget
3 participants