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

Optional Media Controls for Chromecast style remote #555

Merged
merged 3 commits into from
Nov 12, 2024

Conversation

prabhjotsbhatia-ca
Copy link
Contributor

Copy link
Owner

@PRProd PRProd left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! 👍🏻

I have a few requests for changes ~

Code style (minor)
line 3719 -

    // Draw Optional MediaControl buttons on CC
    function drawMediaControlButtons(e, config) {
      if (config.show_media_controls==true) {
        return html`
          <button class="remote-button" id="rewind-button" @pointerdown=${e.buttonDown}>
            <ha-icon icon="mdi:rewind"></ha-icon>
          </button>
          <button class="remote-button${playingStatusClass}" id="playpause-button" @pointerdown=${e.buttonDown}>
            <ha-icon icon="mdi:play-pause"></ha-icon>
          </button>
          <button class="remote-button" id="fastforward-button" @pointerdown=${e.buttonDown}>
            <ha-icon icon="mdi:fast-forward"></ha-icon>
          </button>
          `;
      }
      return;
    }

return nothing; removed - (error/critical)
Code style (minor)
line 8375 -

  getChromecastMediaControls(remoteStyle) {
    if (['CC1', 'CC2', 'CC3'].includes(remoteStyle)) {
        return html`
          <label for="showMediaControlsCheckbox">
            <input type="checkbox" id="showMediaControlsCheckbox" name="show_media_controls"
              ?checked=${this._config.show_media_controls === true} @change=${this.configChanged}
            >&nbsp; ${this.translateToUsrLang("Show Media Controls")}
          </label>
          <br>
          <br>`;}
  }

Logic correction - using the config value only triggers the checkbox to appear if a chromecast remote is selected as an override. It does not appear if a chromecast remote is being shown when Remote Style: is set to default. Using getDeviceAttribute("defaultRemoteStyle") will return the visible remote style (bad name for that function, I know... sorry)
UI linebreaks (minor)
Line 8502 -

        <br>
        <br>
        ${this.getChromecastMediaControls(getDeviceAttribute("defaultRemoteStyle"))}

        ${this.getCompatibilityModeDropdown(this._config.compatibility_mode, getDeviceAttribute('friendlyName'))}

        ${this.getAppChoiceOptionMenus(getDeviceAttribute("defaultRemoteStyle"))}

@prabhjotsbhatia-ca
Copy link
Contributor Author

Thank you for your review and all the work you put into maintaining this repo! Yes, I was a little confused by the getDeviceAttribute("defaultRemoteStyle").

I have made the suggested changes.

Copy link
Owner

@PRProd PRProd left a comment

Choose a reason for hiding this comment

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

Thanks!

@PRProd
Copy link
Owner

PRProd commented Nov 11, 2024

I'll complete the merge right before the next official release. I like to keep my repo updates a little more "transactional" where changes are immediately available in releases. It helps me keep my sanity. 😄

@prabhjotsbhatia-ca
Copy link
Contributor Author

No worries, totally understandable!
Lemme know if you'd like some help with other issues.

@PRProd
Copy link
Owner

PRProd commented Nov 11, 2024

@prabhjotsbhatia-ca ~

If you have a Fire TV device or a Shield, there are some things you could dig into here: Technical Help Wanted #115

@prabhjotsbhatia-ca
Copy link
Contributor Author

Ah I do have a firtev stick but unfortunately the remote got misplaced. Anything else I could help with?

@PRProd PRProd merged commit 4caa17b into PRProd:main Nov 12, 2024
1 check passed
@PRProd
Copy link
Owner

PRProd commented Nov 12, 2024

Thanks again @prabhjotsbhatia-ca!

I just merged this PR and bundled it with the newly published Firemote version v4.1.3

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.

[Feature Request]: Optional Media control buttons for remote layouts that lack them e.g. Chromecast
2 participants