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

Link generation is done by HtmlHelper::link() added escape and escapeTit... #17

Merged
merged 3 commits into from
Jul 6, 2014

Conversation

redd
Copy link

@redd redd commented Jun 27, 2014

...le options.

redd referenced this pull request Jun 27, 2014
@torifat
Copy link
Owner

torifat commented Jun 27, 2014

Thanks for the pull request but the test is failing.

@redd
Copy link
Author

redd commented Jun 27, 2014

Fixed

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.79%) when pulling 065fe88 on redd:master into fa0a604 on torifat:master.

@torifat
Copy link
Owner

torifat commented Jun 27, 2014

Awesome 👍 , but do we need both alt & title?

@redd
Copy link
Author

redd commented Jun 27, 2014

The title was duplicated on <img> so I've removed it. Title stays on <a> element as it was earlier, I didn't touch it.

@coveralls
Copy link

Coverage Status

Coverage decreased (-1.79%) when pulling eb345b0 on redd:master into fa0a604 on torifat:master.

@redd
Copy link
Author

redd commented Jul 6, 2014

Do you need anything else?

@torifat
Copy link
Owner

torifat commented Jul 6, 2014

@redd No, I just wanted to have a look at it. But, Sorry I was very busy.

@@ -1102,7 +1102,7 @@ public function testImageMenu() {
'</li',
'<li',
array('a' => array('title' => 'Item 2', 'href' => '/item-2')),
array('img' => array('src' => '/path/my-image.jpg', 'alt' => 'Item 2', 'title' => 'Item 2')),
Copy link
Owner

Choose a reason for hiding this comment

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

@redd I was little bit concerned about this one. This means the change will break the current versions. Right?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but title is reduntant when you've alt on image and title on the link.

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, I agree with you that it is redundant. But, before pushing this change I have to add a how to upgrade section. Or, many existing users will end up having a broken site.

Copy link
Author

Choose a reason for hiding this comment

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

I think that (eventually) don't showing title is a little abuse to have broken sites :) But when image is in the link it should without problems show link's title.

Copy link
Owner

Choose a reason for hiding this comment

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

👍 Sorry, I forgot what it is 😄

torifat added a commit that referenced this pull request Jul 6, 2014
Link generation is done by HtmlHelper::link() added escape and escapeTit...
@torifat torifat merged commit a4befe2 into torifat:master Jul 6, 2014
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

3 participants