Enable the subtitle settings panel for the new layout #655

stonko1994 · 2024-12-09T19:00:16Z

Description

The current version of the new UI layout does not support subtitle customization settings.

Changes

In order to add support for the subtitle customization settings I had to refactor quite a bit inside the SettingsPanel. The changes are outlined in detail below.

Components no longer are separated into modern and non-modern. The name of the components now reflect their purpose.
- ModernSettingsPanelItem was refactored and split into a DynamicSettingsPanelItem and a SettingsPanelSelectOption instead of handling both in the same component.
- The ModernSettingsPanelPage and ModernSettingsPanel were removed and their functionality was moved into the parent components.
The SubtitleSettingsPanelPage now supports the old and the new layout.

Checklist (for PR submitter and reviewers)

CHANGELOG entry A generic redesign changelog with all changes will be added at the end

…gs as well

…ern-subtitle-settings-panel # Conflicts: # src/scss/demo.scss # src/scss/skin-super-modern/components/_settingspanelpage.scss

…functionality in the default ones

stonko1994 · 2024-12-10T16:47:29Z

src/ts/components/button.ts

+    buttonElement.on('click', (e) => {
+      e.stopPropagation();


This is needed to prevent the event from bubbling up to the parent element.

Example: In the new design, the whole SettingsPanelItem is clickable. The subtitle customization item also contains a button to a different page. Without this change, both would trigger when clicking the button.

stonko1994 · 2024-12-10T16:50:00Z

src/ts/components/component.ts

-  protected mergeConfig<Config>(config: Config, defaults: Config, base: Config): Config {
+  protected mergeConfig<Config>(config: Config, defaults: Partial<Config>, base: Config): Config {


Not every component can fill all of its default config values. It does not need to fill out all as others are required anyway on initialization. To improve the type handling I decided to make this Partial here. The alternative (which was used before) was a cast to the Config type.

stonko1994 · 2024-12-10T16:56:47Z

src/ts/components/seekbar.ts

+    // TODO: the problem is that the SeekbarLabel is also inside the seekbarContainer and therefore reacts to the
+    // TODO: mouse inputs.


Will be handled in a follow up PR.

stonko1994 · 2024-12-10T16:58:05Z

src/ts/components/settingspanel.ts

-      this.getDomElement().on('mouseenter', () => {
-        // On mouse enter clear the timeout
-        this.hideTimeout.clear();
+      this.getDomElement().on('mouseenter mousemove', () => {


I had to add the mousemove event here as well because the focusout event triggers between SettingsPanelPage transitions causing the settings panel to hide after selecting anything.

stonko1994 · 2024-12-10T16:59:51Z

src/ts/components/settingspanel.ts

+    if (config.pageTransitionAnimation) {
+      const handleResize = () => {
+        // Reset the dimension of the settingsPanel to let the browser calculate the new dimension after resizing
+        this.getDomElement().css({ width: '', height: '' });
+      };
+
+      player.on(player.exports.PlayerEvent.PlayerResized, handleResize);
+    }


There is a workaround within the animation handling where the width and height of the SettingsPanel are set to the current dimension. This breaks the SettingsPanel when resizing the browser window. Removing the width and height properties on resize fixes this.

stonko1994 · 2024-12-10T17:00:10Z

src/ts/components/settingspanel.ts

+  addPage(page: SettingsPanelPage) {
+    this.addComponent(page);
+    this.updateComponents();
+  }


Convenience method to add a SettingsPanelPage element.

stonko1994 · 2024-12-10T17:01:15Z

src/ts/components/settingspanel.ts

-    targetPage.onActiveEvent();
    sourcePage.onInactiveEvent();
+    targetPage.onActiveEvent();


The previous order was 'wrong' and caused an issue with the new dynamic SettingsPanelPage approach, where the previous page would get the inactive event after the new page got the active event.

stonko1994 · 2024-12-10T17:01:56Z

src/ts/components/settingspanelitem.ts

  };

-  constructor(label: LocalizableText | Component<ComponentConfig>, setting: Component<ComponentConfig>, config: ContainerConfig = {}, addSettingAsComponent: boolean = true) {


I refactored the constructor to follow our config-based approach, which we follow for almost all other Components.

stonko1994 · 2024-12-10T17:03:19Z

src/ts/components/settingspanelitem.ts

  };

-  constructor(label: LocalizableText | Component<ComponentConfig>, setting: Component<ComponentConfig>, config: ContainerConfig = {}, addSettingAsComponent: boolean = true) {
+  constructor(config: SettingsPanelItemConfig)


This second constructor overload is a workaround for the weak-type-detection issue when there is no property of the parent type used.

Not adding this requires to cast the config to a SettingsPanelItemConfig on every usage which is cumbersome.

…ew skin

… new layout

stonko1994 · 2024-12-10T19:02:01Z

src/ts/components/modernsettingspanelitem.ts

Was refactored to the DynamicSettingsPanelItem

stonko1994 · 2024-12-10T19:06:28Z

src/scss/skin-super-modern/components/_settingspanelpage.scss

@@ -6,92 +6,6 @@
  &.#{$prefix}-active {
    display: block;
  }
-
-  // A "line" in the panel: a container holding a label + control


The styling for a settings-panel-item was extracted into a separate file.

stonko1994 · 2024-12-10T19:08:18Z

src/ts/components/subtitlesettings/subtitlesettingspanelpage.ts

  settingsPanel: SettingsPanel;
  overlay: SubtitleOverlay;
+  useDynamicSettingsPanelItem?: boolean;


We still don't know how we want to ship the new UI layout. We are currently leaning towards a new major version. Once this decision is confirmed I'll rework this and just change to the new dynamic approach and throw out the config option again. For now I didn't want to create a subclass for this as the code is very similar.

…e height

src/ts/components/settingspanelitem.ts

…tle-settings-panel

….com:bitmovin/bitmovin-player-ui into feature/super-modern-subtitle-settings-panel

stonko1994 added 8 commits December 6, 2024 10:35

refactor the modern settings panel to add support for subtitle settin…

5ff17c4

…gs as well

clear hide timeout on mouse move

ea9fc09

add a min gap between elements in a settings panel item

0c0e9fd

remove dynamic pages from DOM after navigating back

545e680

replace hardcoded back text with localization

64dad45

improve typing for optional settings

6f21cbf

cleanup duplicate css for settings panel items

cf3ff8a

enable the subtitle settings for the new ui layout

5530ae4

stonko1994 mentioned this pull request Dec 9, 2024

Enable the new UI design on browsers #654

Merged

1 task

stonko1994 added 5 commits December 9, 2024 14:31

Merge branch 'feature/super-modern-ui-for-web' into feature/super-mod…

3461a30

…ern-subtitle-settings-panel # Conflicts: # src/scss/demo.scss # src/scss/skin-super-modern/components/_settingspanelpage.scss

add empty line at EOF

d1ca093

remove the ModernSettingsPanel and ModernSettingsPanelPage and merge …

ebd13e4

…functionality in the default ones

rename modern settings component to dynamic

f2b581b

improve settingspanelitem initialization and configuration

9fb9cc0

stonko1994 commented Dec 10, 2024

View reviewed changes

stonko1994 added 6 commits December 10, 2024 10:08

improve subtitle example overlay access

0d9e490

add documentation

9c6c20f

remove outdated todo

9626fed

minor code style improvements

a0497f5

add support to include the SubtitleSettingsPanelPage in the old and n…

0dff63a

…ew skin

hide the eco mode container behind the ecoMode flag when creating the…

ad708ce

… new layout

stonko1994 commented Dec 10, 2024

View reviewed changes

src/ts/components/modernsettingspanelitem.ts Outdated

Copy link

Collaborator Author

stonko1994 Dec 10, 2024

Choose a reason for hiding this comment

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

Was refactored to the DynamicSettingsPanelItem

stonko1994 changed the title ~~Enable the subtitle settings panel~~ Enable the subtitle settings panel for the new layout Dec 10, 2024

stonko1994 commented Dec 10, 2024

View reviewed changes

fix failing tests

b2e0324

stonko1994 marked this pull request as ready for review December 10, 2024 19:21

stonko1994 added 2 commits December 10, 2024 19:11

add missing removing from dynamic settings panel pages

38f83ec

add line-height to the settings panel item to ensure all have the sam…

86ff8c6

…e height

Base automatically changed from feature/super-modern-ui-for-web to feature/modern-ui-base December 11, 2024 16:34

wasp898 approved these changes Dec 19, 2024

View reviewed changes

src/ts/components/settingspanelitem.ts Outdated Show resolved Hide resolved

stonko1994 added 4 commits December 19, 2024 15:57

Merge branch 'feature/modern-ui-base' into feature/super-modern-subti…

93047fe

…tle-settings-panel

rename setting to settingComponent

59a7bf5

Merge branch 'feature/super-modern-subtitle-settings-panel' of github…

ae1838a

….com:bitmovin/bitmovin-player-ui into feature/super-modern-subtitle-settings-panel

fix test build after renaming

901be17

stonko1994 merged commit 77d2044 into feature/modern-ui-base Dec 20, 2024
3 checks passed

stonko1994 deleted the feature/super-modern-subtitle-settings-panel branch December 20, 2024 07:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable the subtitle settings panel for the new layout #655

Enable the subtitle settings panel for the new layout #655

stonko1994 commented Dec 9, 2024 •

edited

Loading

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

stonko1994 Dec 10, 2024

		protected mergeConfig<Config>(config: Config, defaults: Config, base: Config): Config {
		protected mergeConfig<Config>(config: Config, defaults: Partial<Config>, base: Config): Config {

		// TODO: the problem is that the SeekbarLabel is also inside the seekbarContainer and therefore reacts to the
		// TODO: mouse inputs.

		};

		constructor(label: LocalizableText \| Component<ComponentConfig>, setting: Component<ComponentConfig>, config: ContainerConfig = {}, addSettingAsComponent: boolean = true) {

Enable the subtitle settings panel for the new layout #655

Enable the subtitle settings panel for the new layout #655

Conversation

stonko1994 commented Dec 9, 2024 • edited Loading

Description

Changes

Checklist (for PR submitter and reviewers)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stonko1994 commented Dec 9, 2024 •

edited

Loading