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

feat: another media custom controls type #566

Merged
merged 17 commits into from
Sep 29, 2023
Merged

Conversation

Kyzyl-ool
Copy link
Contributor

  • New custom control type
  • Some simplifications in ReactPlayer
  • Added positioning, ratio and muteButtonHidden props for video

@Kyzyl-ool Kyzyl-ool marked this pull request as draft September 15, 2023 12:56
@gravity-ui-bot
Copy link
Contributor

Preview is ready.

@Kyzyl-ool Kyzyl-ool force-pushed the feat/media-new-controls-type branch from 01deaf8 to ca82669 Compare September 19, 2023 09:25
@Kyzyl-ool Kyzyl-ool marked this pull request as ready for review September 19, 2023 09:25
@Kyzyl-ool Kyzyl-ool force-pushed the feat/media-new-controls-type branch 3 times, most recently from a7be789 to 6dad315 Compare September 27, 2023 09:37
@@ -69,12 +93,37 @@ $controlSize: 64px;
outline: none;
}
}

&_with-uikit-play-pause-button {
width: 42px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

create variable pls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

0 2px 8px 0 var(--g-color-sfx-shadow-light);

&:focus {
outline: 2px solid var(--g-color-line-misc);
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe create mixin for outlines if it would be the same for elements?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, I will create this mixin here #576

width: 24px;
&_type {
&_with-play-pause-button {
height: 24px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

variable

width: 24px;
}
&_with-uikit-play-pause-button {
height: 16px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

variable

@@ -87,6 +136,11 @@ $controlSize: 64px;
height: 24px;
width: 24px;
}
&_with-uikit-play-pause-button {
height: 16px;
Copy link
Collaborator

Choose a reason for hiding this comment

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

variable

mute?: MuteConfigProps;
elapsedTimePercent?: number;
type?: CustomControlsType;
isPaused?: boolean;
onPlayClick?: () => void;
shown: boolean;
Copy link
Collaborator

Choose a reason for hiding this comment

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

make it non required pls

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done


const playPauseButton = useMemo(() => {
if (type !== CustomControlsType.WithPlayPauseButton) {
const icon = isPaused ? playIcon : pauseIcon;
Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like in the end we need only playPauseButton and muteButton, I propose to join all of this useMemo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

left as it is, but removed useMemo from the above variables

wrapper: !currentHeight,
controls,
'background-shadow-rendered':
!backgroundShadowHidden &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

it would be better to use flag showBackgroundShadow or just backgroundShadow

same for muteButtonHidden

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

{
wrapper: !currentHeight,
controls,
'background-shadow-rendered':
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the meaning of 'background-shadow-rendered' modifier?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed, as it is not needed anymore

@Kyzyl-ool Kyzyl-ool force-pushed the feat/media-new-controls-type branch from 6dad315 to 77cecfb Compare September 29, 2023 10:06
@@ -3,18 +3,44 @@

$block: '.#{$ns}CustomBarControls';
$controlSize: 64px;
// custom controls sizes
$with-uikit-play-pause-control-size: 42px;
Copy link
Collaborator

@gorgeousvlad gorgeousvlad Sep 29, 2023

Choose a reason for hiding this comment

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

could you please turn it to camelCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

gorgeousvlad
gorgeousvlad previously approved these changes Sep 29, 2023
@Kyzyl-ool Kyzyl-ool merged commit 8b7ca7d into main Sep 29, 2023
3 checks passed
@Kyzyl-ool Kyzyl-ool deleted the feat/media-new-controls-type branch September 29, 2023 14:40
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