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

Problems with empty attributes #137

Closed
richtera opened this issue Aug 5, 2021 · 5 comments · Fixed by #138
Closed

Problems with empty attributes #137

richtera opened this issue Aug 5, 2021 · 5 comments · Fixed by #138

Comments

@richtera
Copy link

richtera commented Aug 5, 2021

// Convert attribute value to boolean attribute if needed

Hi the way this detects truthy attributes does make sense, but since the definition for the RawProperties doesn't allow for boolean values, it gets type casted to a string with the value "true"

export type RawAttributes = {

This is causing problems parsing and rendering some things. For example
<img alt="" src="Something"/> will end up with alt="true" and if you substribute a react component on top it will complain that alt="true' ie. Warning: Received truefor a non-boolean attributealt``
Thanks
Andy

@pveyes
Copy link
Owner

pveyes commented Aug 5, 2021

The return value of mapAttribute is Attributes type, not RawAttribute, and it includes boolean.

type Attributes = Record<string, number | string | boolean | StyleObject>;
This is also internal API that's not exposed so I don't know how it's relevant

This is causing problems parsing and rendering some things. For example

will end up with alt="true" and if you substribute a react component on top it will complain that alt="true' ie. Warning: Received truefor a non-boolean attributealt``

Can you elaborate more on this? I think there's a snippet of code that's get removed by GH because you're not using a code block.

As for "true" value, it should be passed as is as a string. You can see in this test PR #138 that it keep the string as is without any warning.

If you can send a failing test case that would be great

@richtera
Copy link
Author

richtera commented Aug 5, 2021

Ok I stand corrected on that, but I think it's still causing a problem.
During debugging I see:
image
And then I still get the failure. I can work around it then, but it does seems weird that if I don't use a custom renderer on img it correctly translates the alt="" to an empty property.
I'll add a workaround ignoring alt === true and removing it from the props.
Something like this for the "transform" component would work:

function Image({ alt, ...props }) {
  const zoomClick = useContext(LightboxContext);
  if (alt !== true) {
    props.alt = alt;
  }
  return <img {...props} onClick={zoomClick} onDoubleClick={zoomClick} />;
}

Feels awkward, but I agree it's not "wrong" and does work.

@pveyes
Copy link
Owner

pveyes commented Aug 5, 2021

Maybe I misunderstood something, but if your HTML is <img alt="" /> the react element equivalent would be <img alt={true} />, or if you're using custom transform, the props.alt would be true.

Based on HTML spec, this is expected behavior. If you don't want to get the warning from React, you need to remove all empty string for non-boolean attribute in the HTML, or using custom transform to remove invalid boolean attribute

@pveyes
Copy link
Owner

pveyes commented Aug 5, 2021

I think I know the issue here, not all attributes are eligible as boolean attribute, so it should not be treated as such

I will find a way to check whether attribute is a boolean attribute without increasing bundle size too much because hardcoding a list is not preferable

@pveyes
Copy link
Owner

pveyes commented Aug 6, 2021

Published as v1.0.1

Thanks for raising this issue!

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 a pull request may close this issue.

2 participants