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

Revert CoreImageAttributes width field is of type String but it should be Float #174

Closed
ChrisWiegman opened this issue Dec 29, 2023 · 3 comments
Labels
type: bug Issue that causes incorrect or unexpected behavior

Comments

@ChrisWiegman
Copy link
Contributor

ChrisWiegman commented Dec 29, 2023

Steps to Reproduce

Check CoreImageAttributes width type in GraphQLi. It is of type String.

Issue

CoreImageAttributes.width is of type String.

Expected Behavior

CoreImageAttributes.width should be of type Float.

Additional Information

Note any other relevant details.

Related ticket:

CoreImageAttributes.width is type String, but should be a Float · Issue #136 · wpengine/wp-graphql-content-blocks
You need to revert the following commit: Bug: CoreImage width attribute resolve throws error. (#130) · wpengine/wp-graphql-content-blocks@28fca4a
However we need to document the failing unit test case and provide a workaround for this error.
The workaround is to use an alias for the attributes field so that GrapgQL can resolve the types without any conflicts.

Image

@ChrisWiegman ChrisWiegman added the type: bug Issue that causes incorrect or unexpected behavior label Dec 29, 2023
@justlevine
Copy link
Contributor

justlevine commented Dec 29, 2023

I believe i noted this in #136, but just in case: the ideal fix from a schema perspective is to make all width/height attributes a CssUnit { value: Int, unit:CssUnitTypeEnum } or similar, where the unit type defaults if there isn't an explicit one.

@ChrisWiegman
Copy link
Contributor Author

@justlevine I'm sure you did. We're in the process of converting from Jira to GitHub for the teams own work and, as much as I tried to catch all the dupes in moving over work, I'm sure there are a few.

@josephfusco
Copy link
Member

Closing this as we can continue tracking this in #136 in order to see the full context of the issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug Issue that causes incorrect or unexpected behavior
Projects
None yet
Development

No branches or pull requests

4 participants