Frontend: com_contact view review #17217

zero-24 · 2017-07-23T20:14:49Z

Summary of Changes

Review frontend com_contact views

Testing Instructions

Install sample testing data and navigate all the com_contact frontend views.

Expected result

All still works

Actual result

The views have cs errors ;)

Documentation Changes Required

None

brianteeman · 2017-07-23T20:18:33Z

components/com_contact/views/category/tmpl/default_items.php

+					<label class="filter-search-lbl element-invisible" for="filter-search">
+						<span class="label label-warning">
+							<?php echo JText::_('JUNPUBLISHED'); ?>
+						</span><?php echo JText::_('COM_CONTACT_FILTER_LABEL') . '&#160;'; ?>


shouldnt the php part be on new line, like line 39 below

brianteeman · 2017-07-23T20:19:27Z

components/com_contact/views/category/tmpl/default_items.php

+					<?php echo $item->event->afterDisplayTitle; ?>
+					<?php echo $item->event->beforeDisplayContent; ?>
+					<?php if ($this->params->get('show_position_headings')) : ?>
+							<?php echo $item->con_position; ?><br />


too many tabs

brianteeman · 2017-07-23T20:19:32Z

components/com_contact/views/category/tmpl/default_items.php

+							<?php echo $item->con_position; ?><br />
+					<?php endif; ?>
+					<?php if ($this->params->get('show_email_headings')) : ?>
+							<?php echo $item->email_to; ?><br />


too many tabs

brianteeman · 2017-07-23T20:19:44Z

components/com_contact/views/category/tmpl/default_items.php

+						<?php echo JText::sprintf('COM_CONTACT_TELEPHONE_NUMBER', $item->telephone); ?><br />
+					<?php endif; ?>
+					<?php if ($this->params->get('show_mobile_headings') && !empty ($item->mobile)) : ?>
+							<?php echo JText::sprintf('COM_CONTACT_MOBILE_NUMBER', $item->mobile); ?><br />


too many tabs

brianteeman · 2017-07-23T20:22:14Z

components/com_contact/views/featured/tmpl/default_items.php

+							</th>
+						<?php endif; ?>
+						<?php if ($this->params->get('show_suburb_headings')) : ?>
+						<th class="item-suburb">


needs extra tab

zero-24 · 2017-07-23T21:08:41Z

Thanks @brianteeman i have just fixed the comments.

Quy · 2017-07-24T01:12:10Z

components/com_contact/views/category/tmpl/default_items.php

+				<?php if ($this->items[$i]->published == 0) : ?>
+					<li class="row-fluid system-unpublished cat-list-row<?php echo $i % 2; ?>">
+				<?php else : ?>
+					<li class="row-fluid cat-list-row<?php echo $i % 2; ?>" >


Remove space ?>" >

zero-24 · 2017-07-24T17:38:42Z

Fixed thanks @Quy

Quy · 2017-08-05T04:49:25Z

components/com_contact/views/categories/tmpl/default_items.php

+		<?php if ($this->params->get('show_empty_categories_cat') || $item->numitems || count($item->getChildren())) : ?>
+			<?php if (!isset($this->items[$this->parent->id][$id + 1])) : ?>
+				<?php $class = ' class="last"'; ?>
+			<?php endif; ?>
 			<div <?php echo $class; ?> >


Remove space before and after <?php echo $class; ?>.

Quy · 2017-08-05T04:52:50Z

components/com_contact/views/category/tmpl/default_children.php

+			<?php if ($this->params->get('show_empty_categories') || $child->numitems || count($child->getChildren())) : ?>
+				<?php if (!isset($this->children[$this->category->id][$id + 1])) : ?>
+					<?php $class = ' class="last"'; ?>
+				<?php endif ?>


Quy · 2017-08-05T05:01:56Z

components/com_contact/views/contact/tmpl/default_address.php

+		<dd>
+			<span class="contact-webpage">
+				<a href="<?php echo $this->contact->webpage; ?>" target="_blank" rel="noopener noreferrer" itemprop="url">
+				<?php echo JStringPunycode::urlToUTF8($this->contact->webpage); ?></a>


Indent and move </a> to the next line.

Quy · 2017-08-05T05:04:08Z

components/com_contact/views/featured/tmpl/default.php

+		<h1><?php echo $this->escape($this->params->get('page_heading')); ?></h1>
+	<?php endif; ?>
+	<?php echo $this->loadTemplate('items'); ?>
+	<?php if ($this->params->def('show_pagination', 2) == 1  || ($this->params->get('show_pagination') == 2 && $this->pagination->pagesTotal > 1)) : ?>


Remove extra space

Quy · 2017-08-06T03:30:06Z

components/com_contact/views/contact/tmpl/default_profile.php

+							<?php break; ?>
+						<?php case 'profile_dob': ?>
+							<?php echo '<dd>' . JHtml::_('date', $profile->text, JText::_('DATE_FORMAT_LC4'), false) . '</dd>'; ?>
+						<?php break; ?>


Heggi93 · 2017-08-21T11:33:45Z

I have not tested this item.

No errors found

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17217.}

Heggi93 · 2017-08-21T11:34:23Z

I have tested this item ✅ successfully on 48824b1

No errors found

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17217.}

Quy · 2017-08-26T04:21:15Z

components/com_contact/views/categories/tmpl/default_items.php

+			<?php if (!isset($this->items[$this->parent->id][$id + 1])) : ?>
+				<?php $class = ' class="last"'; ?>
+			<?php endif; ?>
+			<div<?php echo $class; ?>>
 			<?php $class = ''; ?>
 				<h3 class="page-header item-title">
 					<a href="<?php echo JRoute::_(ContactHelperRoute::getCategoryRoute($item->id, $item->language)); ?>">
 					<?php echo $this->escape($item->title); ?></a>


Indent and move </a> to next line. Do the same for line 34.

ghost · 2017-08-27T05:49:36Z

@zero-24 can you please Update Instruction which Codestyle-Errors are meant so Tester know what to look for?

Quy · 2017-08-27T13:27:33Z

components/com_contact/views/featured/tmpl/default.php

-		<?php echo $this->pagination->getPagesLinks(); ?>
-	</div>
-<?php endif; ?>
+	<?php if ($this->params->get('show_page_heading') != 0 ) : ?>


Remove space after 0

Quy · 2017-08-27T13:29:43Z

components/com_contact/views/featured/tmpl/default_items.php

+						<?php endif; ?>
+						<?php if ($this->params->get('show_country_headings')) : ?>
+							<td class="item-state" itemprop="address" itemscope itemtype="https://schema.org/PostalAddress">
+								<span itemprop="addressCountry"><?php echo $item->country; ?></span>


Split into multiple lines? If yes, do the same in the above lines?

zero-24 · 2017-08-29T18:48:36Z

@franz-wohlkoenig please explain what you want from me? I'm a bit lost with your comment? :(

Quy · 2017-08-30T23:48:11Z

components/com_contact/views/contact/tmpl/default.php

 			</h2>
 		</div>
 	<?php endif; ?>
-
-	<?php $show_contact_category = $tparams->get('show_contact_category'); ?>
-
 	<?php if ($show_contact_category === 'show_no_link') : ?>
 		<h3>
 			<span class="contact-category"><?php echo $this->contact->category_title; ?></span>


Multiple lines. Same for lines 49-50.

Quy · 2017-08-30T23:51:35Z

components/com_contact/views/contact/tmpl/default.php

 		<?php echo $this->loadTemplate('address'); ?>
-
 		<?php if ($tparams->get('allow_vcard')) : ?>
 			<?php echo JText::_('COM_CONTACT_DOWNLOAD_INFORMATION_AS'); ?>
 			<a href="<?php echo JRoute::_('index.php?option=com_contact&amp;view=contact&amp;id=' . $this->contact->id . '&amp;format=vcf'); ?>">


Multiple lines.

why? This line looks good to me?

line 105: Indent and move </a> to the next line.

Quy · 2017-08-31T00:03:07Z

components/com_contact/views/contact/tmpl/default_profile.php

@@ -8,37 +8,34 @@
 */

 defined('_JEXEC') or die;
+
 ?>
 <?php if (JPluginHelper::isEnabled('user', 'profile')) :
 	$fields = $this->item->profile->getFieldset('profile'); ?>


Coding style

Quy · 2017-08-31T00:03:51Z

components/com_contact/views/contact/tmpl/default_profile.php

+					<?php echo '<dt>' . $profile->label . '</dt>'; ?>
+					<?php $profile->text = htmlspecialchars($profile->value, ENT_COMPAT, 'UTF-8'); ?>
+					<?php switch ($profile->id) : 
+						 case 'profile_website': ?>


Coding style?

the first switch case can not be written different sadly :(

Quy · 2017-08-31T23:23:54Z

I have tested this item ✅ successfully on 2efc080

Code review

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/17217.}

zero-24 · 2017-11-07T22:56:40Z

Same here I'm closing this as it is conflicting and this is going to be moved against 4.0-dev in a later PR. Thanks @Quy , @franz-wohlkoenig , @Heggi93 & @brianteeman your comments they are not going to be missing. I'm going to implement as much as possible directly in the new PR. Thanks for all the work on the reviews 👍 ❤️

com_contact view review

9782d77

joomla-cms-bot added the PR-staging label Jul 23, 2017

brianteeman reviewed Jul 23, 2017

View reviewed changes

thnaks @brianteeman

998d708

Quy reviewed Jul 24, 2017

View reviewed changes

some smaler missing thanks thanks @Quy

f905a32

thanks @laoneo

0bda48f

Quy reviewed Aug 5, 2017

View reviewed changes

thanks @Quy

307d99d

Quy reviewed Aug 6, 2017

View reviewed changes

zero-24 added 9 commits August 13, 2017 16:42

thanks @Quy

c1d812e

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

dd99ba1

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

7f517fd

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

f19528b

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

56c6c4c

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

979369a

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

7293a32

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

b6fbe21

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

48824b1

Quy reviewed Aug 26, 2017

View reviewed changes

thanks @Quy

b5236eb

Quy reviewed Aug 27, 2017

View reviewed changes

thanks @Quy

0c7e3a2

zero-24 added 3 commits August 29, 2017 20:57

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

b7e5ff5

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

2ca863a

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

db296f8

Quy reviewed Aug 30, 2017

View reviewed changes

Quy reviewed Aug 31, 2017

View reviewed changes

zero-24 added 3 commits August 31, 2017 20:07

thanks @Quy

c79c30d

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

9c2b265

thanks @Quy

2efc080

zero-24 added 9 commits September 2, 2017 11:41

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

2966ded

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

59a4cab

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

da00bb3

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

2a01142

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

0d1242b

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

c34ba15

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

fdb3812

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

dce4479

Merge branch 'staging' of github.com:joomla/joomla-cms into com_contact

55df668

zero-24 closed this Nov 7, 2017

zero-24 deleted the com_contact branch November 7, 2017 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Frontend: com_contact view review #17217

Frontend: com_contact view review #17217

zero-24 commented Jul 23, 2017

brianteeman Jul 23, 2017

brianteeman Jul 23, 2017

brianteeman Jul 23, 2017

brianteeman Jul 23, 2017

brianteeman Jul 23, 2017

zero-24 commented Jul 23, 2017

Quy Jul 24, 2017

zero-24 commented Jul 24, 2017

Quy Aug 5, 2017 •

edited

Quy Aug 5, 2017

Quy Aug 5, 2017

Quy Aug 5, 2017

Quy Aug 6, 2017

Heggi93 commented Aug 21, 2017

Heggi93 commented Aug 21, 2017

Quy Aug 26, 2017 •

edited

ghost commented Aug 27, 2017

Quy Aug 27, 2017

Quy Aug 27, 2017

zero-24 commented Aug 29, 2017

Quy Aug 30, 2017

Quy Aug 30, 2017

zero-24 Aug 31, 2017

Quy Aug 31, 2017

zero-24 Aug 31, 2017

Quy Aug 31, 2017

Quy Aug 31, 2017

zero-24 Aug 31, 2017

Quy commented Aug 31, 2017

zero-24 commented Nov 7, 2017

Frontend: com_contact view review #17217

Frontend: com_contact view review #17217

Conversation

zero-24 commented Jul 23, 2017

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zero-24 commented Jul 23, 2017

Choose a reason for hiding this comment

zero-24 commented Jul 24, 2017

Quy Aug 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Heggi93 commented Aug 21, 2017

Heggi93 commented Aug 21, 2017

Quy Aug 26, 2017 • edited

Choose a reason for hiding this comment

ghost commented Aug 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zero-24 commented Aug 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Quy commented Aug 31, 2017

zero-24 commented Nov 7, 2017

Quy Aug 5, 2017 •

edited

Quy Aug 26, 2017 •

edited