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

Replace unnecessary double quotes in administrator #3 #13295

Merged
merged 5 commits into from
Dec 25, 2016
Merged

Replace unnecessary double quotes in administrator #3 #13295

merged 5 commits into from
Dec 25, 2016

Conversation

frankmayer
Copy link
Contributor

Summary of Changes

  • Replace unnecessary double quotes

The changes in this PR should be fairly easy to review. In hope that this will get merged quickly so further work can be done without conflicting with other PRs. ;)

Testing Instructions

None, should not change behavior

Documentation Changes Required

None.

@@ -119,7 +119,7 @@ public function renderMenu($id = 'menu', $class = '')
// Recurse through children if they exist
while ($this->_current->hasChildren())
{
echo "<ul " . $id . " " . $class . ">\n";
echo '<ul ' . $id . ' ' . $class . ">\n";
Copy link
Contributor

Choose a reason for hiding this comment

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

hum why don't we use PHP_EOL const here ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Depending on OS PHP_EOL sends \n (linux) or \r\n (windows) so unfortunately it's not doable...

Copy link
Contributor

Choose a reason for hiding this comment

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

ok

@@ -214,20 +214,20 @@ public function renderLevel($depth)

if ($this->_current->link != null && $this->_current->target != null)
{
echo "<a" . $linkClass . " " . $dataToggle . " href=\"" . $this->_current->link . "\" target=\"" . $this->_current->target . "\" >"
. $this->_current->title . $dropdownCaret . "</a>";
echo '<a' . $linkClass . ' ' . $dataToggle . " href=\"" . $this->_current->link . "\" target=\"" . $this->_current->target . "\" >"
Copy link
Contributor

Choose a reason for hiding this comment

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

why not use it in href and target too and remove the slashes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wanted to leave that for a rainy day... Want PR's to go through as smoothly as possible ;)

Copy link
Contributor

Choose a reason for hiding this comment

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

well in this case i think this should be part of this PR. just saying ...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, ok then.. will do those as well... :)

}
elseif ($this->_current->link != null && $this->_current->target == null)
{
echo "<a" . $linkClass . " " . $dataToggle . " href=\"" . $this->_current->link . "\">" . $this->_current->title . $dropdownCaret . "</a>";
echo '<a' . $linkClass . ' ' . $dataToggle . " href=\"" . $this->_current->link . "\">" . $this->_current->title . $dropdownCaret . '</a>';
Copy link
Contributor

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as above ;)

'rssitemdesc' => 1,
'word_count' => 200,
'cache' => 0,
'moduleclass_sfx' => ' list-striped');
Copy link
Contributor

Choose a reason for hiding this comment

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

cs: please move the ); to the next line.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do, thanks.

@@ -122,7 +122,7 @@
<script src="<?php echo $this->baseurl; ?>/templates/<?php echo $this->template; ?>/js/template.js"></script>
<!--[if lt IE 9]><script src="<?php echo JUri::root(true); ?>/media/jui/js/html5.js"></script><![endif]-->
</head>
<body class="admin <?php echo $option . " view-" . $view . " layout-" . $layout . " task-" . $task . " ";?>" data-spy="scroll" data-target=".subhead" data-offset="87">
<body class="admin <?php echo $option . ' view-' . $view . ' layout-' . $layout . ' task-' . $task . ' ';?>" data-spy="scroll" data-target=".subhead" data-offset="87">
Copy link
Contributor

Choose a reason for hiding this comment

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

the last space sounds really useful :)

@@ -646,8 +646,8 @@ public static function versions($typeAlias, $itemId, $height = 800, $width = 500
public static function modal($targetModalId, $icon, $alt)
{
$title = JText::_($alt);
$dhtml = "<button data-toggle='modal' data-target='#" . $targetModalId . "' class='btn btn-small'>
<span class='" . $icon . "' title='" . $title . "'></span> " . $title . "</button>";
$dhtml = '<button data-toggle="modal" data-target="# ' . $targetModalId . '" class="btn btn-small">
Copy link
Contributor

Choose a reason for hiding this comment

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

data-target="# ' - this is not correct, you add an extra space here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! Yes ;)

@@ -214,20 +214,20 @@ public function renderLevel($depth)

if ($this->_current->link != null && $this->_current->target != null)
{
echo "<a" . $linkClass . " " . $dataToggle . " href=\"" . $this->_current->link . "\" target=\"" . $this->_current->target . "\" >"
. $this->_current->title . $dropdownCaret . "</a>";
echo '<a' . $linkClass . ' ' . $dataToggle . ' href="' . $this->_current->link . '" target="' . $this->_current->target . '" >'
Copy link
Contributor

Choose a reason for hiding this comment

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

this extra space is not needed
Please replase '" >' to '">' as it is done below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, done.

@shur
Copy link
Contributor

shur commented Dec 24, 2016

I have tested this item ✅ successfully on a9cf8ed

Now is good!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/13295.

@wilsonge wilsonge merged commit 787b6d5 into joomla:staging Dec 25, 2016
@frankmayer frankmayer deleted the unnecessary-double-quotes-in-administrator-no-3 branch December 25, 2016 21:26
@zero-24 zero-24 added this to the Joomla 3.7.0 milestone Dec 26, 2016
@shur
Copy link
Contributor

shur commented Dec 27, 2016

@frankmayer
Copy link
Contributor Author

@shur Thank you for the link :) It's very nice to get a personal acknowledgement. But, lets not forget the community effort involved, to get any PR finally merged.

So, thank you team Joomla, for constructive criticism, testing and improving my commits (and me).

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.

6 participants