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

NSFW - Love thy layout - NSFW #4397

Closed
wants to merge 8 commits into from
Closed

NSFW - Love thy layout - NSFW #4397

wants to merge 8 commits into from

Conversation

nternetinspired
Copy link
Contributor

Give layouts the love they deserve

Layouts are awesome. There's currently a layout for article info-blocks, the definition list, but it is only currently being used in the category blog view. That makes me sad.

This PR standardises com_content views by extending the use of the definition list layout to article and featured article views as well. As intended with layouts this improves readability, maintainability and versatility of views as the article info definition list markup exists now only in one place, not three.

Testing

There should be no observable changes. A useless div that previously existed in the article view is now removed, but that has no affect on rendered views, it was superflous.

Expected result

An improvement in code readability and maintainability.

Signed-off-by: Seth Warburton <seth@internet-inspired.com>
Signed-off-by: Seth Warburton <seth@internet-inspired.com>
Signed-off-by: Seth Warburton <seth@internet-inspired.com>
@nternetinspired nternetinspired changed the title Standardise use of layouts Love thy layout - NSFW Oct 2, 2014
@nternetinspired nternetinspired changed the title Love thy layout - NSFW NSFW - Love thy layout - NSFW Oct 2, 2014
@wilsonge
Copy link
Contributor

@test unsuccessful. Got a PHP error:

( ! ) Notice: Undefined variable: useDefList in JOOMLA_ROOT\components\com_content\views\article\tmpl\default.php on line 35

Signed-off-by: Seth Warburton <seth@internet-inspired.com>
@nternetinspired
Copy link
Contributor Author

Sorry @wilsonge, that should be fixed now.

Happy New Year matey :)

@infograf768
Copy link
Member

This works OK here.
One more test.

<?php endif; ?>
</dd>
<?php endif; ?>
<?php // Todo Not that elegant would be nice to group the params ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just a duplicate with your latest change (sorry be to an arse)

@wilsonge
Copy link
Contributor

wilsonge commented Jan 2, 2015

Happy new year dude :)

Signed-off-by: Seth Warburton <seth@internet-inspired.com>
@nternetinspired
Copy link
Contributor Author

Great catch @wilsonge 🙇

@infograf768
Copy link
Member

Thanks.

wilsonge added a commit that referenced this pull request Mar 20, 2015
Regression: #4397 dropped tags when info block at the bottom
wilsonge added a commit that referenced this pull request Apr 4, 2015
Regression: #4397 dropped tags when info block at the bottom
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