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 menu captions to the TV UI #459

Open
wants to merge 29 commits into
base: develop
Choose a base branch
from
Open

Add menu captions to the TV UI #459

wants to merge 29 commits into from

Conversation

kishorens
Copy link
Contributor

@kishorens kishorens commented Jun 28, 2022

Jira - https://bitmovin.atlassian.net/browse/SOL-3687

Checklist (for PR submitter and reviewers)

  • CHANGELOG entry

@kishorens kishorens requested a review from dweinber July 4, 2022 15:26
Copy link
Member

@dweinber dweinber 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 working on this! The code looks good (apart from a few nitpicks about code style / white spaces).

I'm afraid we'll need to tweak the functionality and design though:

  1. When the MenuCaption gets displayed, the SettingsToggleButtons get pushed down. I don't think we should do that, the buttons must not move.
  2. The MenuCaption should be displayed when the SettingsToggleButton has the focus (e.g. when navigating via TAB there on desktop) as well as when the button is clicked (= menu is opened). The first part is not in place yet.

src/ts/components/menucaption.ts Outdated Show resolved Hide resolved
src/ts/components/settingstogglebutton.ts Outdated Show resolved Hide resolved
src/ts/uifactory.ts Outdated Show resolved Hide resolved
src/ts/uifactory.ts Outdated Show resolved Hide resolved
kishorens and others added 4 commits July 8, 2022 11:12
Co-authored-by: Daniel Weinberger <[email protected]>
Co-authored-by: Daniel Weinberger <[email protected]>
Base automatically changed from feature/smarttv-ui-poc to develop December 14, 2022 15:56
@dweinber
Copy link
Member

I've made a few changes to this PR:

  • It uses the existing Label component instead of creating a new one
  • It is shown when the SettingsToggleButton is focused (e.g. hovered or selected (but not activated) via spatial navigation) and hidden if not focused
  • Position is now below the buttons, so that hide/show doesn't trigger layout changes.

@felix-hoc would appreciate if you could review this one.

Screenshots:
Screenshot 2024-06-28 at 15 23 22

Screenshot 2024-06-28 at 15 23 16

Screenshot 2024-06-28 at 15 23 26

Screenshot 2024-06-28 at 15 23 30

@bit-jacoba let me know if you're happy with that.

@dweinber dweinber requested review from dweinber and removed request for dweinber June 28, 2024 13:47
@dweinber
Copy link
Member

(Need to check why the CI failed 🤔)

@dweinber
Copy link
Member

dweinber commented Jul 1, 2024

(CI failure was just a tslint error, which is fixed now)

Copy link
Contributor

@felix-hoc felix-hoc left a comment

Choose a reason for hiding this comment

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

Implementation looks OK from my side, just not sure about the design/concept. It feels a bit "unfinished". Perhaps we could at least center the labels underneath the buttons? But the Audio Track one might be a bit too long.

Alternatively, what if we just had the label displayed when the menu is opened? So as a header to the select box.

@dweinber
Copy link
Member

I changed the approach quite a bit as I realized that all the buttons already have span/label elements included. There's now a new (optional) config option for all buttons (including ToggleButton, SettingsToggleButton etc) to show the label on hover/focus.

However, I'm still not 100% happy with this, as long texts could be cut off for buttons on the right or left side, but I don't have a better idea.

This is how it looks like now:

Screenshot 2024-07-10 at 12 45 38

Screenshot 2024-07-10 at 12 45 21

Screenshot 2024-07-10 at 12 45 32

Copy link
Contributor

@felix-hoc felix-hoc left a comment

Choose a reason for hiding this comment

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

Nice, IMO this approach is better 👍🏼

Could we also get rid of the blue glow in the background of the text?

src/scss/skin-modern/_skin-tv.scss Outdated Show resolved Hide resolved
src/ts/components/button.ts Outdated Show resolved Hide resolved
@dweinber
Copy link
Member

Could we also get rid of the blue glow in the background of the text?

I really tried but didn't manage. Happy to receive any suggestions or help 😅

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.

3 participants