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 empty alt attribute rendering #58

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

3Qax
Copy link

@3Qax 3Qax commented Jan 2, 2021

This is my first OSS contribution, so excuse me if I've done anything incorrectly.

After performing site scan with Lighthouse, I found that alt attributes with empty values aren't being rendered, despite it being correct value:

However, a decorative image that isn't discussed by the surrounding text but still has some relevance can be included in a page using the img element. Such images are decorative, but still form part of the content. In these cases, the alt attribute must be present but its value must be the empty string.

According to Lighthouse it negatively impacts SEO. I am not sure if the approach that I chose is the right one - extra condition in render() like

guard let value = value, !value.isEmpty else {
    if (Context.self == HTML.ImageContext.self && name == "alt")  {
        return #"alt="""#
    } else {
         return ignoreIfValueIsEmpty ? "" : name
    }
}

would also do the job, though it doesn't feel right. Also, since my solution is breaking change, I am wondering if I should keep the old init, depreciate it and make it call new init like:

@available(*, deprecated, message: "Please use init(name:_,value:_,ifValueIsEmpty:_) instead.")
public init(name: String,
            value: String?,
            ignoreIfValueIsEmpty: Bool = true) {
   self.init(name: name, value: value, ifValueIsEmpty: ignoreIfValueIsEmpty ? .ignore : .renderJustName)
}

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.

1 participant