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

[4.0] HTML class naming standard [frontend][com-content] #20863

Merged
merged 3 commits into from
Jun 27, 2018

Conversation

ciar4n
Copy link
Contributor

@ciar4n ciar4n commented Jun 25, 2018

Pull Request for Issue #15279 .

Summary of Changes

Adds HTML classes using the following standard to com-content frontend views.

ExtensionType-Extension(-SubExtension)(-View) (Eg. 'com-content-category-blog`).

Note: If the default view then -View is omitted. If only one SubExtension exists or if the SubExtension matches the Extension then -SubExtension is omitted.

Child views and child elements within the views

BEM class naming is adopted..

ExtensionType-Extension__Element.

For B/C, old classes remain.

Testing Instructions

Code review.

Expected result

All works fine

Actual result

All works fine

Documentation Changes Required

Yes

<?php if ($this->params->get('show_category_heading_title_text', 1) == 1) : ?>
<h3> <?php echo JText::_('JGLOBAL_SUBCATEGORIES'); ?> </h3>
<?php endif; ?>
<?php echo $this->loadTemplate('children'); ?> </div>
<?php endif; ?>
<?php if (($this->params->def('show_pagination', 1) == 1 || ($this->params->get('show_pagination') == 2)) && ($this->pagination->pagesTotal > 1)) : ?>
<div class="w-100">
<div class="com-content-category-blog__navigation w-100">
Copy link
Contributor

Choose a reason for hiding this comment

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

The markup here is slight different than the following. So should it be changed to match where navigation should be pagination?

https://github.com/joomla/joomla-cms/pull/20863/files#diff-d7f1c49f9242c56fa8126070edfdc9feR275

		<div class="com-content-category__pagination w-100">
 			<?php if ($this->params->def('show_pagination_results', 1)) : ?>
 				<p class="counter float-right pt-3 pr-2">
 					<?php echo $this->pagination->getPagesCounter(); ?>
 				</p>
 			<?php endif; ?>
 
 			<?php echo $this->pagination->getPagesLinks(); ?>
</div>

Copy link
Contributor Author

@ciar4n ciar4n Jun 26, 2018

Choose a reason for hiding this comment

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

So this container currently contains both the pagination and the page counter. I opted to call the container 'navigation' and then name both the pagination and counter separately. This was an after thought so I may need to refactor some of the previous PRs to match. Alternatively we can bundled both counter and pagination as one and place the 'pagination' class to the container as you suggest?

Copy link
Contributor

Choose a reason for hiding this comment

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

I will leave it up to decide which is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now lets leave as is. I'm sure if ideas like #20623 get implemented, this will most likely get refactored in the future.

@@ -272,7 +272,7 @@
<?php // Add pagination links ?>
<?php if (!empty($this->items)) : ?>
<?php if (($this->params->def('show_pagination', 2) == 1 || ($this->params->get('show_pagination') == 2)) && ($this->pagination->pagesTotal > 1)) : ?>
<div class="w-100">
<div class="com-content-category__pagination w-100">
Copy link
Contributor

@Quy Quy Jun 26, 2018

Choose a reason for hiding this comment

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

Apply the same changes with __navigation, __counter, and __pagination as here: https://github.com/joomla/joomla-cms/pull/20863/files#diff-1752f352424eccafe6af03cb5983a2b2R123

$class = 'last';
}
?>
<div class="<?php echo $class; ?>" >
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove space before >

@Quy
Copy link
Contributor

Quy commented Jun 27, 2018

I have tested this item ✅ successfully on 4aab132


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

@laoneo laoneo added this to the Joomla 4.0 milestone Jun 27, 2018
@laoneo laoneo merged commit 9c86ac8 into joomla:4.0-dev Jun 27, 2018
@ciar4n ciar4n deleted the class-naming-com-content branch June 27, 2018 08:30
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.

None yet

4 participants