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

Unify Control readOnly property case to camelCase #1810

Merged
merged 8 commits into from
Aug 3, 2022
Merged

Conversation

mkrecek234
Copy link
Contributor

@mkrecek234 mkrecek234 commented Aug 2, 2022

change readonly to readOnly

@mkrecek234 mkrecek234 requested a review from mvorisek August 2, 2022 14:33
@mvorisek mvorisek removed their request for review August 2, 2022 14:39
src/Form/Control.php Outdated Show resolved Hide resolved
@mvorisek
Copy link
Member

mvorisek commented Aug 2, 2022

@mkrecek234 what is your opinion on readonly? It seems it is one word keyword used in LC across many languages (PHP, C#, TS, ...).

@mkrecek234
Copy link
Contributor Author

mkrecek234 commented Aug 2, 2022

@mvorisek you can argue that "readonly" is common for many languages, but many languages don't camelCase. Thus, I clearly prefer the camelCase as it is according to the rules. In English, proper writing is "read-only", which also supports to camelCase it. Any other opinion @abbadon1334 @ibelar ? Not a major topic, but let's make ATK4 even more beautiful ;-)

@@ -136,7 +136,7 @@ public function getInput()
'type' => $this->inputType,
'id' => $this->name . '_input',
'value' => $this->getValue(),
'readonly' => $this->readonly ? 'readonly' : false,
'readOnly' => $this->readOnly ? 'readonly' : false,
Copy link
Member

Choose a reason for hiding this comment

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

this is definitely wrong, readonly is canonical form of HTML attribute

@mvorisek mvorisek changed the title CamelCase readOnly control property Unify Control readOnly property case to camelCase Aug 3, 2022
@mvorisek
Copy link
Member

mvorisek commented Aug 3, 2022

@mvorisek you can argue that "readonly" is common for many languages, but many languages don't camelCase. Thus, I clearly prefer the camelCase as it is according to the rules. In English, proper writing is "read-only", which also supports to camelCase it. Any other opinion @abbadon1334 @ibelar ? Not a major topic, but let's make ATK4 even more beautiful ;-)

In atk4/* the camelCase naming is defined by the "most natural snake_case". Both "read_only" and "readonly" variants make sense (vs. "checkbox", almost noone would write "check_box").

I have done more reseach on "readonly" vs "readOnly" with Google. When I was renaming the property in atk4/data, I did not think much of it and simply converted the snake_case to camelCase. The case used in other projects in quite mixed, LC is used mainly when it is a "language attribute" which is almost always LC (case insensitive) only. When used as a property name, they are projects which use it like proposed. To unify the naming with atk4/data (atk4 ecosystem in general), I am ok with this change.

@mvorisek mvorisek merged commit e316b6c into develop Aug 3, 2022
@mvorisek mvorisek deleted the fix_readOnly branch August 3, 2022 03:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants