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

Allow specifying the padding of the individual sprites in the group selector as well #201

Merged
merged 3 commits into from Jun 4, 2020

Conversation

papandreou
Copy link
Member

@papandreou papandreou self-assigned this May 24, 2020
@papandreou papandreou requested a review from Munter May 24, 2020 21:04
await expect(spriteAsset.rawSrc, 'to have metadata satisfying', {
size: {
width: 12,
height: 90,
Copy link
Member

Choose a reason for hiding this comment

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

I'm a bit surprised about height 90. But I guess that would be padding behavior. I was more expecting a margin collapsing behavior, resulting in 50 being the result here.

I would expect if I set the padding property, I get that amount of empty space around each sprite. But I don't need double that. Since empty space is equal no matter if I'm looking at it from the perspective of one sprite or the other it should be fine. But I can see how that might be difficult to express for a sprite packer

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I was also a bit 🤔 about it, but this is not really changing how the packers work, it just adds another way to apply it to a set of images.

I think I remember that the "Jim Scott" packer doesn't/can't easily support padding, but the horizontal and vertical packers should be easy to get to work the way you describe with collapsing of padding etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

@Munter, should we get this in and revisit the packer behavior separately?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. This is better than before. Lets merge

@papandreou papandreou merged commit 62f4170 into master Jun 4, 2020
@papandreou papandreou deleted the feature/paddingInGroupSelector branch June 4, 2020 15:04
@papandreou
Copy link
Member Author

Released in 3.2.0, update assetgraph-builder to it and released 8.0.1 of that.

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 this pull request may close these issues.

None yet

2 participants