I'm not a php dev, so what do you all think of this patch

JaredBusch

I just submitted this as a pull request to FreePBX's User Manager module.

I think it looks clean, but curious to what someone with more day to day PHP skills thinks about it.

diff --git a/views/welcome.php b/views/welcome.php
index af9ee0c..ba9e8ec 100644
--- a/views/welcome.php
+++ b/views/welcome.php
@@ -31,19 +31,19 @@
 							<div role="tabpanel" id="users" class="tab-pane display active">
 								<div class="table-responsive">
 									<div id="toolbar-users">
+										<a href="<?php echo _(sizeof($directories)==1 ? '?display=userman&action=adduser&directory=1' : '#'); ?>" id="add-users" class="btn btn-add" data-type="users" data-section="users"<?php echo _(sizeof($directories)==1 ? '':' disabled title="Select Directory to enable \'Add\' Button"')?>>
-										<a href="#" id="add-users" class="btn btn-add" disabled data-type="users" data-section="users" title="Select Directory to enable 'Add' Button">
 											<i class="fa fa-user-plus"></i> <span><?php echo _('Add')?></span>
 										</a>
+										<button id="remove-users" class="btn btn-danger<?php echo _(sizeof($directories)==1 ? ' btn-remove':''); ?>" disabled data-type="users" data-section="users"<?php echo _(sizeof($directories)==1 ? '':' disabled title="Select Directory to enable \'Delete\' Button"')?>>
-										<button id="remove-users" class="btn btn-danger" data-type="users" disabled data-section="users" title="Select Directory to enable 'Delete' Button">
 											<i class="fa fa-user-times"></i> <span><?php echo _('Delete')?></span>
 										</button>
 										<button id="email-users" class="btn btn-info btn-send" data-type="users" disabled data-section="users">
 											<i class="fa fa-envelope-o"></i> <span><?php echo _('Send Email')?></span>
 										</button>
 										<select id="directory-users" class="form-control" style="display: inline-block;width: inherit;">
+											<option value=""><?php echo _('All Directories')?></option>
-											<option value=""><?php echo _("All Directories")?></option>
 											<?php foreach($directories as $directory) {?>
+												<option value="<?php echo $directory['id']?>"<?php echo _(sizeof($directories)==1 ? ' selected':'')?>><?php echo $directory['name']?></option>
-												<option value="<?php echo $directory['id']?>"><?php echo $directory['name']?></option>
 											<?php } ?>
 										</select>
 									</div>
@@ -68,16 +68,16 @@
 								<div class="table-responsive">
 									<div class="alert alert-info"><?php echo _("Group Priorities can be changed by clicking and dragging groups around in the order you'd like. Groups with a lower number for priority take priority (EG 0 is higher than 1)")?></div>
 									<div id="toolbar-groups">
+										<a href="config.php?display=userman&amp;action=addgroup" id="add-groups" class="btn btn-add" data-type="groupss" data-section="groups"<?php echo _(sizeof($directories)==1 ? '':' disabled title="Select Directory to enable \'Add\' Button"')?>>
-										<a href="config.php?display=userman&amp;action=addgroup" id="add-groups" class="btn btn-add hidden" data-type="groupss" data-section="groups">
 											<i class="fa fa-user-plus"></i> <span><?php echo _('Add')?></span>
 										</a>
+										<button id="remove-groups" class="btn btn-danger btn-remove" data-type="groups" data-section="groups"<?php echo _(sizeof($directories)==1 ? 'disabled':' disabled title="Select Directory to enable \'Delete\' Button"')?>>
-										<button id="remove-groups" class="btn btn-danger btn-remove hidden" data-type="groups" disabled data-section="groups">
 											<i class="fa fa-user-times"></i> <span><?php echo _('Delete')?></span>
 										</button>
 										<select id="directory-groups" class="form-control" style="display: inline-block;width: inherit;">
 											<option value=""><?php echo _("All Directories")?></option>
 											<?php foreach($directories as $directory) {?>
+												<option value="<?php echo $directory['id']?>"<?php echo _(sizeof($directories)==1 ? " selected":'')?>><?php echo $directory['name']?></option>
-												<option value="<?php echo $directory['id']?>"><?php echo $directory['name']?></option>
 											<?php } ?>
 										</select>
 									</div>

1337

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

I just submitted this as a pull request to FreePBX's User Manager module.

I think it looks clean, but curious to what someone with more day to day PHP skills thinks about it.

Context matters when it comes to coding style. Do you have a link to the original source code?

JaredBusch

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

I just submitted this as a pull request to FreePBX's User Manager module.

I think it looks clean, but curious to what someone with more day to day PHP skills thinks about it.

Context matters when it comes to coding style. Do you have a link to the original source code?

The actual git is public as long as you are logged in to Attlasian.
https://git.freepbx.org/projects/FREEPBX/repos/userman/browse
They have a copy on Github ~~but that is no longer kept in sync~~ (apparently it is).
https://github.com/FreePBX/userman

1337

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

I just submitted this as a pull request to FreePBX's User Manager module.

I think it looks clean, but curious to what someone with more day to day PHP skills thinks about it.

Context matters when it comes to coding style. Do you have a link to the original source code?

The actual git is public as long as you are logged in to Attlasian.
https://git.freepbx.org/projects/FREEPBX/repos/userman/browse
They have a copy on Github ~~but that is no longer kept in sync~~ (apparently it is).
https://github.com/FreePBX/userman

Your code looks fine. It looks just like the original.

https://git.freepbx.org/projects/FREEPBX/repos/userman/browse/views/welcome.php

JaredBusch

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

I just submitted this as a pull request to FreePBX's User Manager module.

I think it looks clean, but curious to what someone with more day to day PHP skills thinks about it.

Context matters when it comes to coding style. Do you have a link to the original source code?

The actual git is public as long as you are logged in to Attlasian.
https://git.freepbx.org/projects/FREEPBX/repos/userman/browse
They have a copy on Github ~~but that is no longer kept in sync~~ (apparently it is).
https://github.com/FreePBX/userman

Your code looks fine. It looks just like the original.

https://git.freepbx.org/projects/FREEPBX/repos/userman/browse/views/welcome.php

It is a pull request, it should.

I was discussing WHAT i did in my change, hence the diff in post 1.
Mostly using the ternary structure like I did.

1337

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

I just submitted this as a pull request to FreePBX's User Manager module.

I think it looks clean, but curious to what someone with more day to day PHP skills thinks about it.

Context matters when it comes to coding style. Do you have a link to the original source code?

The actual git is public as long as you are logged in to Attlasian.
https://git.freepbx.org/projects/FREEPBX/repos/userman/browse
They have a copy on Github ~~but that is no longer kept in sync~~ (apparently it is).
https://github.com/FreePBX/userman

Your code looks fine. It looks just like the original.

https://git.freepbx.org/projects/FREEPBX/repos/userman/browse/views/welcome.php

It is a pull request, it should.

I was discussing WHAT i did in my change, hence the diff in post 1.
Mostly using the ternary structure like I did.

Yeah, that was what I was commenting. It's fine.

In general if...then...else is clearer than ? to read but it also clearer to not put many php tags back to back. But that is how the original is done. So what you've done is inline with how it is written.

1337

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

I just submitted this as a pull request to FreePBX's User Manager module.

I think it looks clean, but curious to what someone with more day to day PHP skills thinks about it.

Context matters when it comes to coding style. Do you have a link to the original source code?

The actual git is public as long as you are logged in to Attlasian.
https://git.freepbx.org/projects/FREEPBX/repos/userman/browse
They have a copy on Github ~~but that is no longer kept in sync~~ (apparently it is).
https://github.com/FreePBX/userman

Your code looks fine. It looks just like the original.

https://git.freepbx.org/projects/FREEPBX/repos/userman/browse/views/welcome.php

It is a pull request, it should.

I was discussing WHAT i did in my change, hence the diff in post 1.
Mostly using the ternary structure like I did.

Yeah, that was what I was commenting. It's fine.

In general if...then...else is clearer than ? to read but it also clearer to not put many php tags back to back. But that is how the original is done. So what you've done is inline with how it is written.

The only very minor thing I could find, would be to not use ' and " in the same statement, unless there is a reason for it.

So instead of:

<?php echo _(sizeof($directories)==1 ? " selected":'')?>

it would probably be better to do:

<?php echo _(sizeof($directories)==1 ? ' selected' : '') ?>

(easier to read when you're consistent with white space as well)

It will work either way so...

JaredBusch

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

@pete-s said in I'm not a php dev, so what do you all think of this patch:

@jaredbusch said in I'm not a php dev, so what do you all think of this patch:

I just submitted this as a pull request to FreePBX's User Manager module.

I think it looks clean, but curious to what someone with more day to day PHP skills thinks about it.

Context matters when it comes to coding style. Do you have a link to the original source code?

The actual git is public as long as you are logged in to Attlasian.
https://git.freepbx.org/projects/FREEPBX/repos/userman/browse
They have a copy on Github ~~but that is no longer kept in sync~~ (apparently it is).
https://github.com/FreePBX/userman

Your code looks fine. It looks just like the original.

https://git.freepbx.org/projects/FREEPBX/repos/userman/browse/views/welcome.php

It is a pull request, it should.

I was discussing WHAT i did in my change, hence the diff in post 1.
Mostly using the ternary structure like I did.

Yeah, that was what I was commenting. It's fine.

In general if...then...else is clearer than ? to read but it also clearer to not put many php tags back to back. But that is how the original is done. So what you've done is inline with how it is written.

The only very minor thing I could find, would be to not use ' and " in the same statement, unless there is a reason for it.

So instead of:
<?php echo _(sizeof($directories)==1 ? " selected":'')?>
it would probably be better to do:
<?php echo _(sizeof($directories)==1 ? ' selected' : '') ?>
(easier to read when you're consistent with white space as well)

It will work either way so...

meant to use ' that was a miss on me.