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

Parser: Round float attributes to check validity #3494

Closed
wants to merge 1 commit into from

Conversation

youknowriad
Copy link
Contributor

When resizing images, they receive a float "height/width" values and these values might defer a bit when the post is saved server side because of the PHP default precision which would cause the image block to appear invalid if you refresh

This PR updates the attributes comparison mechanism to round float attributes before comparing them.

Testing instructions

  • Create an image block
  • Resize the image
  • Save and refresh
  • The block shouldn't show up as invalid
  • Repeat several times :)

@youknowriad youknowriad added the [Type] Bug An existing feature does not function as intended label Nov 15, 2017
@youknowriad youknowriad self-assigned this Nov 15, 2017
@youknowriad youknowriad requested review from mtias and aduth November 15, 2017 12:28
@youknowriad youknowriad changed the title Parser: Rount float attributes to check validity Parser: Round float attributes to check validity Nov 15, 2017
@codecov
Copy link

codecov bot commented Nov 15, 2017

Codecov Report

Merging #3494 into master will increase coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3494      +/-   ##
==========================================
+ Coverage   34.63%   34.68%   +0.04%     
==========================================
  Files         260      260              
  Lines        6738     6741       +3     
  Branches     1219     1219              
==========================================
+ Hits         2334     2338       +4     
+ Misses       3718     3717       -1     
  Partials      686      686
Impacted Files Coverage Δ
blocks/api/validation.js 96.92% <100%> (+1.76%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4e68f11...75d461b. Read the comment docs.

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

I tested this with many case in images resized to very very small sizes with float sizes in height and I did not found any regression. The code also looks good to me. So I think this is ok to be merged.
But I think it would be ok to round the values to integer sizes, we are talking about pixels so I think using float values is not needed.

@youknowriad
Copy link
Contributor Author

closing this and going with #3495 for now. We should avoid recreating the block comments server-side in the future.

@youknowriad youknowriad deleted the fix/float-attributes-dirty-checking branch November 15, 2017 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants