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

fix(views): only output subtitle element in object summary if provided #9829

Closed
wants to merge 1 commit into from

Conversation

jdalsem
Copy link
Member

@jdalsem jdalsem commented May 21, 2016

fixes #9759

@@ -49,7 +49,9 @@
if ($title_link) {
echo "<h3>$title_link</h3>";
}
echo "<div class=\"elgg-subtext\">$subtitle</div>";
if ($subtitle) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe should still output an empty string if set to ''?

Copy link
Member

Choose a reason for hiding this comment

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

User can use (one blank space) or<!-- --> to get an empty element if that's what you mean.

@hypeJunction
Copy link
Contributor

Commit message type should be fix

@mrclay
Copy link
Member

mrclay commented May 21, 2016

While our BC policy doesn't guarantee markup in views it would be nice to add a bit about the effect of this in guides/upgrading.rst. Someone's layout CSS could depend on that element always being present.

@mrclay
Copy link
Member

mrclay commented May 21, 2016

fix in the summary, Fixes in the description. I'd be in favor of altering our system to accept this commit as is.

@jdalsem jdalsem changed the title fixed(views): only output subtitle element in object summary if provided fix(views): only output subtitle element in object summary if provided May 23, 2016
@jdalsem
Copy link
Member Author

jdalsem commented May 23, 2016

I have updated the commit message. I feel it is overkill to add info about this change in the upgrade documentation

@mrclay
Copy link
Member

mrclay commented May 23, 2016

I think 2.x is the place for this change, but I won't block it. We should be reserving 1.12 and 2.1 for clear cut bugs and this is just an opinion about empty markup being present.

@hypeJunction
Copy link
Contributor

Do you still want this in 1.12, or is #9944 enough?

@jdalsem
Copy link
Member Author

jdalsem commented Jul 22, 2016

#9944 is fine by me

@jdalsem jdalsem closed this Jul 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants