-
Notifications
You must be signed in to change notification settings - Fork 172
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
Adding code comments. #4289
Adding code comments. #4289
Conversation
… to match order in other functions.
@@ -61,7 +61,6 @@ protected static function createField(string $field, array $field_json_metadata, | |||
'#type' => 'textfield', | |||
'#required' => TRUE, | |||
'#title' => t('Identifier'), | |||
'#default_value' => $field_json_metadata['identifier'] ?? '', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious as to why this was removed, possibly it wasn't necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya it was duplicate as there is another line in the same array setting the #default_value
to the variable, see https://github.com/GetDKAN/dkan/pull/4289/files/beb8bbad16647ffa8fc03efef5b8782d297ae26b#diff-3d012ae0c7bc8097d5d36c410d4df6d57c3c89124b4ddc1ee1bd5015afd08e7aL66
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for adding the comments for better code clarity. It's hard to tell where some items are removed whether or not they we're leftover/unused variables but I think QA of the feature branch will unearth that if necessary, merging this to the feature branch with respect to the fact that we have a QA ticket for feature functionality coverage. Thanks!
re: 20743
Describe your changes
Adding code comments for #4185, also:
QA Steps