ML
    • Recent
    • Categories
    • Tags
    • Popular
    • Users
    • Groups
    • Register
    • Login

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

    IT Discussion
    2
    8
    214
    Loading More Posts
    • Oldest to Newest
    • Newest to Oldest
    • Most Votes
    Reply
    • Reply as topic
    Log in to reply
    This topic has been deleted. Only users with topic management privileges can see it.
    • JaredBuschJ
      JaredBusch
      last edited by 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>
      1 1 Reply Last reply Reply Quote 0
      • 1
        1337 @JaredBusch
        last edited by

        @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?

        JaredBuschJ 1 Reply Last reply Reply Quote 0
        • JaredBuschJ
          JaredBusch @1337
          last edited by

          @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

          1 1 Reply Last reply Reply Quote 0
          • 1
            1337 @JaredBusch
            last edited by

            @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

            JaredBuschJ 1 Reply Last reply Reply Quote 0
            • JaredBuschJ
              JaredBusch @1337
              last edited by

              @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.

              1 1 Reply Last reply Reply Quote 0
              • 1
                1337 @JaredBusch
                last edited by

                @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.

                1 1 Reply Last reply Reply Quote 0
                • 1
                  1337 @1337
                  last edited by 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...

                  JaredBuschJ 1 Reply Last reply Reply Quote 0
                  • JaredBuschJ
                    JaredBusch @1337
                    last edited by

                    @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.

                    1 Reply Last reply Reply Quote 0
                    • 1 / 1
                    • First post
                      Last post