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

Feature: Mute Button #172

Merged
merged 3 commits into from
Aug 3, 2019
Merged

Feature: Mute Button #172

merged 3 commits into from
Aug 3, 2019

Conversation

uetchy
Copy link
Contributor

@uetchy uetchy commented Jul 15, 2019


name: Pull request
about: Create a pull request to contribute in making Pock amazing!
title: ''
labels: ''
assignees: ''


Describe the Pull request

  • Add mute button
  • Fix index calculation
let index = Int(ceil(location.x / (segmentedControl.frame.width / CGFloat(controls.count)))) - 1
  • Replace volumeItems with new copies of each buttons
    • Long-pressing mute button without other volume controls can make slider icons vanished

Screenshots
Touch Bar Shot 2019-07-15 at 20 55 53

Fixes
A comma-separated list of issues that can be closed with this PR.
[e.g. #1, #2, #3 (*required)]
#155

Pull request type
Keep only the option that better matches your pull request.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so I can reproduce.
Please, remember to fill the Test Configuration section with requested details.

Test Configuration:

  • Hardware: [e.g. MacBook Pro, 15 (2018) (*required)
  • macOS Version: [e.g. 10.14.5 (*required)]
  • Xcode version: [e.g. 10.2.1 (*required)]

Checklist
Use this list to keep track of your progress:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works

@@ -90,4 +90,5 @@ extension Defaults.Keys {
static let shouldShowBrightnessUpItem = Defaults.Key<Bool>("shouldShowBrightnessUpItem", default: true)
static let shouldShowVolumeDownItem = Defaults.Key<Bool>("shouldShowVolumeDownItem", default: true)
static let shouldShowVolumeUpItem = Defaults.Key<Bool>("shouldShowVolumeUpItem", default: true)
static let shouldShowToggleMuteItem = Defaults.Key<Bool>("shouldShowToggleMuteItem", default: true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it is better to have this option false by default.

case is CCVolumeUpItem, is CCVolumeDownItem:
slideableController?.set(downItem: volumeItems.first, upItem: volumeItems.last)
case is CCVolumeUpItem, is CCVolumeDownItem, is CCToggleMuteItem:
slideableController?.set(downItem: CCVolumeUpItem(parentWidget: self), upItem: CCVolumeDownItem(parentWidget: self))
Copy link
Collaborator

Choose a reason for hiding this comment

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

downItem should be CCVolumeDownItem and upItem should be CCVolumeUpItem 🌚

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed now🦄

Copy link
Collaborator

@pigigaldi pigigaldi left a comment

Choose a reason for hiding this comment

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

I added a few comments for some little things I think should be addressed before I can merge this excellent PR 👍

@pigigaldi pigigaldi merged commit 9d83101 into pock:master Aug 3, 2019
@uetchy uetchy deleted the mute-button branch August 3, 2019 15:53
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