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

Add predefined size dropdown in the save panel #49

Closed

Conversation

@ShamsAlDinShakuntala
Copy link

@ShamsAlDinShakuntala ShamsAlDinShakuntala commented Jan 17, 2019

Add predefined size dropdown.

go2

Fixes #36


IssueHunt Summary

IssueHunt has been backed by the following sponsors. Become a sponsor

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

Nice work! This is a great start :)

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

Since this is in the save panel, we can't have the two group boxes vertically aligned. Instead they should be laid out horizontally.

Like this (just quick mockup):
screen shot 2019-01-17 at 17 10 24

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

Would be nice to support percent in addition to pixels in the pixels dropdown.

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

Fit into => Dimensions

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

The Width and Height fields should be limited to the size of the input video. The fields should also only accept integers.

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

The Dimensions dropdown should mark the original size:

480 x 270 (Original)

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

Not all of the labels are vertically aligned with the components (dropdown/textfield).

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

Maybe we should also show the percentage in the Dimensions dropdown? That would make it easier to understand how much smaller from the original the sizes are.

Example:

480 x 270 (Original)
240 x 135 (50%)

Loading

@sindresorhus sindresorhus changed the title Add predefined size dropdown. Add predefined size dropdown in the save panel Jan 17, 2019
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

Would be very cool if when the width/height input is focused, if I pressed up/down arrow keys, it would increase/decrease the value. Probably has to be done as a NSTextField subclass.

Loading

@ShamsAlDinShakuntala
Copy link
Author

@ShamsAlDinShakuntala ShamsAlDinShakuntala commented Jan 17, 2019

Many thanks for the feedback! I'll revise the pull request to:

  • Horizontally align the group boxes
  • Add support for percent in addition to pixels
  • Add input validation for the width and height fields
  • Mark the original size with (Original), show percentage in the Dimensions dropdown
  • Vertically align all labels with components
  • Allow incrementing/decrementing width or height by pressing on up/down arrow
  • Use property NSLockLockedTemplate and NSLockUnlockedTemplate instead of raw string values
  • Rename Fit into into Dimensions

I'm not quite sure what you meant by:
> Fit into => Dimensions
Are you referring to renaming the Custom dropdown menu item into Dimensions, or adding another label Dimensions to better clarify the dropdown's purpose?

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 17, 2019

Fit into => Dimensions
Are you referring to renaming the Custom dropdown menu item into Dimensions, or adding another label Dimensions to better clarify the dropdown's purpose?

Sorry, I should have been clearer. I just meant renaming the Fit into label to Dimensions.

Loading

Gifski/SavePanelLockView.swift Outdated Show resolved Hide resolved
Loading
@ShamsAlDinShakuntala
Copy link
Author

@ShamsAlDinShakuntala ShamsAlDinShakuntala commented Jan 18, 2019

I've updated the pull request to incorporate all the above-mentioned changes:

  • Horizontally align the group boxes
  • Add support for percent in addition to pixels
  • Add input validation for the width and height fields
  • Mark the original size with (Original), show percentage in the Dimensions dropdown
  • Vertically align all labels with components
  • Allow incrementing/decrementing width or height by pressing on up/down arrow
  • Use property NSLockLockedTemplate and NSLockUnlockedTemplate instead of raw string values
  • Rename Fit into into Dimensions

Screenshots for input validation under both pixel mode and percent mode:
Input validation under pixel mode
Input validation under percent mode

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 18, 2019

  • Can you make the group boxes centered in the save panel? Right now, if you resize the save panel, they're locked to the right. They should stay in the middle.
  • I don't think showing alert dialogs when the input is invalid is very friendly. Instead I think we should just revert input value to the closest max/min value. Would be very cool to show a shake animation of the textfield when that happens.
  • The star emoji used in the Dimension dropdown is a bit too large. Using some kind of icon would probably be better. Or maybe we should just drop the icon at all? Is it really useful?
  • The lock icon view should not be tab'able. If I press Tab in the save panel, it first goes to dimensions dropdown, then width input, then height input, then the lock icon (which has no focus state).
  • The controls in the "Other Settings" group could use a tiny bit more vertical spacing between them.
  • Pressing up/down arrow in the text input is working great, but it's a chore to change it to a much lower number. Would you be able to add support for a modifier? So if Shift is pressed while pressing up/down, it will increase/decrease by 10 instead of 1.

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 18, 2019

I just realized something. In contrast to apps that save images, we don't support saving unproportioned GIFs, and I don't see the point in that either, so I think the whole aspect lock thing is moot, unfortunately... I should have been more clear in #36 that the demo GIF there was just for inspiration and was not meant to be taken exactly as is.

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 18, 2019

You don't need to squash and force push. I will do that when merging anyway. Just add new commits. That makes it easier for me to review what changed.

Loading

}
}

func control(_ control: NSControl, textView: NSTextView, doCommandBy commandSelector: Selector) -> Bool {
Copy link
Owner

@sindresorhus sindresorhus Jan 18, 2019

Choose a reason for hiding this comment

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

Can you move this logic out into a NSTextField subclass called IntTextField that handles the validation and arrow up/down logic? It could also have settings for minimum and maximum value.

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Jan 28, 2019

@ShamsAlDinShakuntala Let me know if you're waiting for feedback from me. Ignore this comment if you're just busy.

Loading

@ShamsAlDinShakuntala
Copy link
Author

@ShamsAlDinShakuntala ShamsAlDinShakuntala commented Feb 16, 2019

Apologies - The past few weeks has been crazily busy for me.

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Feb 16, 2019

No worries at all. I just wanted to make sure you were not waiting for me.

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Mar 10, 2019

@ShamsAlDinShakuntala Friendly ping :)

Loading

@sindresorhus sindresorhus force-pushed the master branch 2 times, most recently from 6c37629 to 7b999da Mar 26, 2019
@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented Apr 10, 2019

Loading

@sindresorhus
Copy link
Owner

@sindresorhus sindresorhus commented May 3, 2019

Closing this to give other people a chance to work on it.

Loading

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

2 participants