feat(css-bkg-pos): Added possibility to declare background pos by single numeric value #7958
Conversation
260d619 to
6899ff2
Compare
|
I'm afraid I'm going to need a little bit of help with the tests for this PR: what I need to do is see if the drawing of the background Canvas/UIImage was done correctly, and I can't figure out how to achieve this. Other options would be to spy on some of the methods that draw the background, but that would not be possible on Android, where all the action happens on Android side in Could anybody please point me in the right direction on how to write tests for this? I tested this manually and all works great. |
vmutafov
left a comment
There was a problem hiding this comment.
I made some comments in the code. Generally speaking, it would be great to reuse code whenever possible and extract nested ifs into separate functions or additional classes in order to achieve better code design thus increasing readability and lowering the maintenance costs.
| res.posX = spaceX; | ||
| } | ||
|
|
||
| if ("center".equals(vy.getString().toLowerCase(Locale.ENGLISH))) { |
There was a problem hiding this comment.
Couldn't this logic be reused with the one below? This if introduces code duplication. Also, perhaps it's a good idea to extract pieces like vy.getString().toLowerCase(Locale.ENGLISH) into separate variables. This would increase readability, easier debugging and would not perform multiple method calls.
There was a problem hiding this comment.
I'm afraid that without strong unit tests around this area, an important part of the parser could brake. I could write those unit tests, but as stated above, I would need some help with that 😞
…gle numeric value (#7958) * feat(css-bkg-pos): Added possibility to declare background pos by single numeric value * feat(css-bkg-pos): Implemented numeric bkg pos for iOS * feat(css-bkg-pos): removed unnecessary code Co-authored-by: Vasil Trifonov <v.trifonov@gmail.com>
PR Checklist
What is the current behavior?
You can't declare a background-position attribute in CSS using only one numeric value:
What is the new behavior?
Now you can declare
background-positionas above mentioned, and additionally in the following format:Implements #4201