chore(): Reorder defaults for fabric object + patch setRelativeXY
#9474
Conversation
- Move InteractiveObject defaults to its file - Move constructor qunit tests to jest - Move FX_DURATION to straighten since that's the only thing using it
|
I was curious what migrating some of the tests from qunit to jest might reveal because of my previous PR. Let me know if this makes sense or is a bit much (i.e., you don't want tests moved over or properties may not have a good order anyway). |
|
This is not wrong imho, but since i don't want to scroll the page up and down and check properties one by one, before adding this i will add a test in master that checks that the end result will be the same. In general there is an issue with reordering things:
so in general i m against diffs in pr that are done for example to reorder imports. |
I understand not wanting to reorder so you don't add history you don't want (and you have to go somewhere else anyway to find the default so may not matter that the order is different). What do you think about moving the defaults that aren't in fabric object? Also about migrating tests? The migration revealed that selectable isn't a part of fabric object but the test was passing because the default is being crammed into there, even though it's a property of a subclass. I can redo the branch and keep the original order but I think the owndefaults should represent what they are. Alternatively, I could have the interactive object's defaults in default values.ts to keep the defaults together but in separate objects so the owndefaults for interactive object is correct. |
There was a problem hiding this comment.
I also understand the diff point
However if we keep it contained to a standalone PR is it so bad
The other side of this point is we don't want to keep the code nt tidy just because of the diff.
The import order for me is a pain. But I let go of it.
Reordering makes sense because:
- The migration was incremental so cleanup is needed, all development is actually
- For accessibility of the code. We know how it is built, but the community doesn't and it makes it harder to contribute
|
migrating tests is welcome |
|
i m adding the test in master so we can merge this if the test pass |
|
@gloriousjob there is some nasty merge conflict. can you fix it? |
|
ok maybe i solved the conflict |
|
I added the test and merged master in, i let the discussion on the best ordering to you guys. |
- also make colorProperties static
Thanks! I'll work with @ShaMan123 for now. Not saying we'll get to the perfect place but hopefully better. Also, I would've fixed the master merge but fine for now. Only thing was trying to change the name of the test suite to match the file. |
There was a problem hiding this comment.
We can have a defaults in each file and export from here a spread of all
export const defualtValues = {
...fillDefaults,
...strokeDefaults,
...transformDefaults
}
what do you think?
There was a problem hiding this comment.
I prefer it because it seems more orderly and easy to maintain/find/navigate
There was a problem hiding this comment.
My concern there would be that the PR was opened so that we could separate the properties of the base Object from the InteractiveObject so that unit tests would work correctly because of correct class encapsulation. Should transformDefaults include the interactive defaults or just be the ones in the base Object?
Also, what about other properties (e.g., is paintFirst a fill or a stroke property? Does objectCaching fall into any of these categories)?
I also got the impression that @asturur liked everything together so orderly may depend on the beholder (although I lean towards your view, @ShaMan123).
There was a problem hiding this comment.
It is a matter a taste, let's try to wrap this up.
paintFirst belongs to rendering props if we have something like that.
Regarding transform props, good point.
There was a problem hiding this comment.
paintFirstbelongs to rendering props if we have something like that.
Maybe next to objectCaching
There was a problem hiding this comment.
I think I’ll leave this alone due to the separation, but you’re welcome to provide a commit as long as it keeps the separation (it kind of would make more sense being in their files since that’s supposed to be the pattern).
The only other thing I’d like in this PR would be to rename the FabricObject in object.ts to BaseObject. This would reduce some confusion on which class is being referenced (the base object versus the one people use from the library). I understand why it was renamed since it’s not good practice to name a class something that JavaScript has as a keyword but duplicating another name seems just as bad. I’ll let you guys decide how to resolve this comment.
There was a problem hiding this comment.
That should be done in a separate PR
I am merging
Good job, glorious ;)
|
Can you rebase/update from master? |
ShaMan123
changed the title
setRelativeXY
|
Can you add a changelog entry? Then I can merge. |
|
Not sure if I did it right but just checked in an additional line. I tried to make it match the lines around it. |
Motivation
Original idea was move fabric object constructor tests into jest. This revealed that the test was testing for a property that FabricObject doesn't even have and led to moving properties for that into InteractiveFabricObject, which did have them. Also created brief constructor test for InteractiveFabricObject for property in that class.
Description
Move InteractiveFabricObject defaults to its proper file so as not to have defaults not typed to FabricObject assigned. Re-arrange the remaining properties by class name and order in the class.
Changes
colorPropertiesto static