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

Use layer name as filename prefix in zip export #786

Merged
merged 2 commits into from
Mar 3, 2018

Conversation

sharpfives
Copy link
Contributor

Hi!

Piskel is such a great project; I use it all the time for making scenes and characters in my video games. Thanks for making this!

I'd like to have the exported layers optionally start with the layer name, rather than the layer index. For most of my games, I'm copying over the exported layers and frames from Piskel into another project, and then referencing them by filename in the game code. If I need to come back to Piskel and add another layer in the middle of others are re-export, my the existing layers' filenames are now shifted due to the index change and my game code's file references are wrong. Optionally using the layer name in the exported files rather than index would allow easier migration of files to other projects that aren't expecting layer file names to change each time.

I added a new checkbox to the zip export html template to enable this option, and supporting logic to the zip export controller. The new checkbox should not show unless the "split by layers" options is checked.

Let me know what you think.

Thanks again,
Alex

…prefix to file names (in ZipExportController) if checkbox is selected.
@juliandescottes
Copy link
Collaborator

Thanks for the nice feedback and for the PR. The idea sounds good, I'll review the change this weekend.

Copy link
Collaborator

@juliandescottes juliandescottes left a comment

Choose a reason for hiding this comment

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

Looks really good and works fine! I have a few suggestions, let me know if you can take care of it otherwise I will update it in a follow up!

Thanks

@@ -11,7 +11,11 @@
<input id="zip-split-layers" class="zip-split-layers-checkbox checkbox-fix" type="checkbox" />
<label for="zip-split-layers">Split by layers</label>
</div>
<div class="use-layer-names-checkbox-container" style="margin: 5px 0;">
Copy link
Collaborator

Choose a reason for hiding this comment

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

We should keep the checkbox-container class here, since it slightly influences the display. This way the text for both checkboxes will be correctly aligned.

Feel free to go with a shorter name for the other class e.g. class="checkbox-container use-layer-names-container"


this.useLayerNamesCheckboxContainer = document.querySelector('.use-layer-names-checkbox-container');
this.useLayerNamesCheckbox = document.querySelector('.zip-use-layer-names-checkbox');
this.useLayerNamesCheckboxContainer.hidden = !this.splitByLayersCheckbox.checked;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we hide this with CSS? Either set style.display set to block or none. Or classList.toggle("hidden", !this.splitByLayersCheckbox.checked);


this.useLayerNamesCheckboxContainer = document.querySelector('.use-layer-names-checkbox-container');
this.useLayerNamesCheckbox = document.querySelector('.zip-use-layer-names-checkbox');
this.useLayerNamesCheckboxContainer.hidden = !this.splitByLayersCheckbox.checked;
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could also get into its own method so that we can call it from here and from onSplitLayersClick_?

@sharpfives
Copy link
Contributor Author

Makes sense! I'll make those changes and push an update. Thanks.

@juliandescottes
Copy link
Collaborator

Looks good, thanks for addressing the comments @sharpfives ! Merging. I'm preparing release v0.14 right now so your change should be available very soon on piskelapp.com :)

@juliandescottes juliandescottes merged commit 64cd724 into piskelapp:master Mar 3, 2018
@sharpfives
Copy link
Contributor Author

@juliandescottes No problem, thanks again for making this. Also nice work keeping the code clean and organized - made it super easy for me to get up and running.

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.

2 participants