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

Fixed invalid HTML markup in drift fonticons #2995

Merged
merged 1 commit into from
Dec 11, 2017

Conversation

skateman
Copy link
Member

@skateman skateman commented Dec 11, 2017

The tag helper method in Rails is a little bit inconsistent when it's about self-closing tags. It works well with an input field, but it fails to work with an i tag that we're using for fonticons. This causes random duplication of tags when displaying a drift history of a host. Forcing the tag to be closed by using an empty content_tag instead solves the issue.

Before:
screenshot from 2017-12-11 15-22-11

After:
screenshot from 2017-12-11 15-21-41

https://bugzilla.redhat.com/show_bug.cgi?id=1466402

@miq-bot add_label bug, gaprindashvili/yes
@miq-bot assign @epwinchell

@miq-bot
Copy link
Member

miq-bot commented Dec 11, 2017

Checked commit skateman@47669cd with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 👍

@@ -1223,7 +1223,7 @@ def drift_add_diff_image(idx, val)

def drift_add_image_col(idx, img_src, img_bkg, val)
html = ViewHelper.content_tag(:div, :class => img_bkg) do
ViewHelper.tag(:i, :class => img_src, :title => val)
ViewHelper.content_tag(:i, nil, :class => img_src, :title => val)
Copy link
Member

Choose a reason for hiding this comment

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

@skateman does it make sense to add a test here?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ohadlevy well, not really. The code is ancient and IMHO it shouldn't even exist. Also the issue is not happening always and it's browser-dependent.

@epwinchell
Copy link
Contributor

@skateman Tested. Looks good.

@epwinchell
Copy link
Contributor

@miq-bot assign @himdel

@miq-bot miq-bot assigned himdel and unassigned epwinchell Dec 11, 2017
@mzazrivec mzazrivec added this to the Sprint 75 Ending Dec 11, 2017 milestone Dec 11, 2017
@mzazrivec mzazrivec merged commit 46531e5 into ManageIQ:master Dec 11, 2017
@skateman skateman deleted the drift-invalid-tag branch December 11, 2017 15:45
simaishi pushed a commit that referenced this pull request Dec 11, 2017
Fixed invalid HTML markup in drift fonticons
(cherry picked from commit 46531e5)

https://bugzilla.redhat.com/show_bug.cgi?id=1524711
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit b3b015953092c51a267fba968fc29b0d6b11d0d0
Author: Milan Zázrivec <[email protected]>
Date:   Mon Dec 11 16:44:15 2017 +0100

    Merge pull request #2995 from skateman/drift-invalid-tag
    
    Fixed invalid HTML markup in drift fonticons
    (cherry picked from commit 46531e5fa5c4b8491e8a3bc0e3d7c0e930dae8f2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1524711

simaishi pushed a commit that referenced this pull request Dec 13, 2017
Fixed invalid HTML markup in drift fonticons
(cherry picked from commit 46531e5)

https://bugzilla.redhat.com/show_bug.cgi?id=1525563
@simaishi
Copy link
Contributor

Fine backport details:

$ git log -1
commit 06aae16b89b0a3abd532f0a67d71463beec8c7fb
Author: Milan Zázrivec <[email protected]>
Date:   Mon Dec 11 16:44:15 2017 +0100

    Merge pull request #2995 from skateman/drift-invalid-tag
    
    Fixed invalid HTML markup in drift fonticons
    (cherry picked from commit 46531e5fa5c4b8491e8a3bc0e3d7c0e930dae8f2)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1525563

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants