new setting button for CBPlayer #768

jurassic-period · 2020-07-29T12:12:49Z

closes #num

codecov-commenter · 2020-07-29T12:17:02Z

Codecov Report

Merging #768 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #768   +/-   ##
=======================================
  Coverage   77.46%   77.46%           
=======================================
  Files          85       85           
  Lines        1522     1522           
=======================================
  Hits         1179     1179           
  Misses        343      343

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3c4646f...dd0bd53. Read the comment docs.

jurassic-period · 2020-07-29T12:22:15Z

@amshkv

amshkv

Все хорошо, но надо переделать (с)

amshkv · 2020-07-29T12:19:02Z

services/app/assets/css/style.scss

+.cb-icon-setting {
+  display: flex;
+  font-size: 17px;
+}


хм
а зачем тут увеличивать размер шрифта?

по дефолту он в rem и равен 16пикс, а ты своим действием ломаешь это

да и флекс родитель в бутстрапе делается чуть по-другому .d-flex
я так и не понял зачем он нам тут

Размер я подбирал чтобы визуально шестерёнка подходила, flex лучше центрировал иконку, без него она почему то вниз слазит, флекс как я думаю, надёжней чем вручную двигать поэтому увидел, что он центрировал и оставил его

дефолт лучше подбора
в таких случаях лучше заюзать что-то стандартное

Плюс ты можешь благодаря флексам сделать полное выравнивание по центру, но это надо делать через бутсраповские классы, а не свои

amshkv · 2020-07-29T12:20:08Z

services/app/assets/js/widgets/components/CBPlayer/ControlPanel.jsx

@@ -72,6 +72,23 @@ const ControlPanel = ({
      </button>
      {children}
      <button type="button" className={speedControlClassNames} onClick={onChangeSpeed}>x2</button>
+      <div className="dropup">


что за класс такой интересный?

Это бутсраповский класс для "Выпадающий вверх".

Верно, извиняюсь, тут пропустил

без него тоже вверх выпадает)) убрал его

amshkv · 2020-07-29T12:21:45Z

services/app/assets/js/widgets/components/CBPlayer/ControlPanel.jsx

@@ -72,6 +72,23 @@ const ControlPanel = ({
      </button>
      {children}
      <button type="button" className={speedControlClassNames} onClick={onChangeSpeed}>x2</button>
+      <div className="dropup">
+        <button
+          className="btn dropdown-toggle cb-icon-setting"


что-то мне подсказывает, что мы можем немного схитрить, убрать dropdown-toggle и не понадобиться то, что ты выдумал с .dropdown-toggle::after

кнопка обычно не существует без каких-то описывающих ее классов-принадлежностей
https://getbootstrap.com/docs/4.5/components/buttons/#examples

Всё верно, так гораздо лучше будет и чище. Я не проверял т.к. решил, что этот класс поддерживает возможность открывать список т.к. без него это будет просто кнопкой, но ошибался.

amshkv · 2020-07-29T12:22:00Z

services/app/assets/css/style.scss

+.dropdown-toggle::after {
+  content: none;
+}


только что сломались все дропдауны на сайте ;)

amshkv · 2020-07-29T12:23:12Z

services/app/assets/js/widgets/components/CBPlayer/ControlPanel.jsx

+          <button type="button" className="dropdown-item" href="/">Action</button>
+          <button type="button" className="dropdown-item" href="/">Another action</button>
+          <button type="button" className="dropdown-item" href="/">Something else here</button>


кнопки у которых есть href? что-то новенькое)

ссылки заменял на кнопки, невнимательность (

amshkv · 2020-07-29T12:27:46Z

services/app/assets/css/style.scss

+.cb-icon-setting:active, .cb-icon-setting:focus {
+  outline: none;
+  box-shadow: none;
+}


x-outline-none есть у ребят в файле стилей, скорее всего тебе его хватит

amshkv · 2020-07-29T13:08:58Z

services/app/assets/js/widgets/components/CBPlayer/ControlPanel.jsx

@@ -72,6 +72,23 @@ const ControlPanel = ({
      </button>
      {children}
      <button type="button" className={speedControlClassNames} onClick={onChangeSpeed}>x2</button>
+      <div>
+        <button
+          className="btn d-flex cb-icon-setting"


для кнопки класс так и не указал никакой дополнительный

до этого у тебя тут было убирание аутлайна, а сейчас и его нет

не совсем понял, тут дополнительный класс - "cb-icon-setting"

Или речь о чём то вроде "btn-info" ? Если да, то для чего это нужно?

речь о чем-то вроде btn-info,
бутстрап так устроен, что он сам делает то что надо и при нажатиях и без них, при фокусе и тд
для этого ему нужно знать тип кнопки, который указывается вторым классом
возможно тебе поможет btn-link или что-то с аутлайном

ты отключаешь тут тень, но аутлайн так и не выключил (хз нужно тебе это или нет)

amshkv · 2020-07-29T13:09:45Z

services/app/assets/js/widgets/components/CBPlayer/ControlPanel.jsx

@@ -72,6 +72,23 @@ const ControlPanel = ({
      </button>
      {children}
      <button type="button" className={speedControlClassNames} onClick={onChangeSpeed}>x2</button>
+      <div>


а нужен ли тут тогда пустой див?
и всё-таки верни то что рекомендует бутстрап
то что ты не видишь что что-то не работает, не значит что это так)
я про dropup

amshkv · 2020-07-29T13:10:16Z

services/app/assets/css/style.scss

@@ -166,6 +166,10 @@ a {
  right: 10%;
 }

+.cb-icon-setting:active, .cb-icon-setting:focus {
+  box-shadow: none;
+}


без этого разве нельзя обойтись?

если бы при нажатии тень не вылазила, а так у меня пока нет идей как по другому отключить дефолтное поведение бустраповской кнопки

Или оставить тень кнопки? Не красиво же смотрится при нажатии..

если хочешь убирать, то надо сделать как класс outline, это класс-утилита и у него должно быть другое название

jurassic-period · 2020-07-29T14:38:40Z

jurassic-period · 2020-07-29T16:12:33Z

new setting button for CBPlayer

95c81b7

amshkv requested changes Jul 29, 2020

View reviewed changes

fixed

d77e428

jurassic-period requested a review from amshkv July 29, 2020 12:56

amshkv previously approved these changes Jul 29, 2020

View reviewed changes

fixed class

b2e576c

jurassic-period dismissed amshkv’s stale review via b2e576c July 29, 2020 13:29

jurassic-period requested a review from amshkv July 29, 2020 13:29

added style for dropap-button

c66e153

jurassic-period added 2 commits July 29, 2020 18:07

removed classes and shadow

c3586c1

use bootstrap classes

aaccdb9

amshkv previously approved these changes Jul 29, 2020

View reviewed changes

button x2 moved inside dropup

dd0bd53

jurassic-period dismissed amshkv’s stale review via dd0bd53 July 30, 2020 16:20

ReDBrother self-requested a review July 30, 2020 17:51

ReDBrother approved these changes Jul 30, 2020

View reviewed changes

ReDBrother merged commit 288453b into hexlet-codebattle:master Jul 30, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

new setting button for CBPlayer #768

new setting button for CBPlayer #768

jurassic-period commented Jul 29, 2020

codecov-commenter commented Jul 29, 2020 •

edited

jurassic-period commented Jul 29, 2020

amshkv left a comment

amshkv Jul 29, 2020

amshkv Jul 29, 2020

amshkv Jul 29, 2020

jurassic-period Jul 29, 2020

amshkv Jul 29, 2020 •

edited

amshkv Jul 29, 2020

jurassic-period Jul 29, 2020

amshkv Jul 29, 2020

jurassic-period Jul 29, 2020

amshkv Jul 29, 2020

amshkv Jul 29, 2020

jurassic-period Jul 29, 2020

amshkv Jul 29, 2020

amshkv Jul 29, 2020

jurassic-period Jul 29, 2020

amshkv Jul 29, 2020

amshkv Jul 29, 2020

amshkv Jul 29, 2020

jurassic-period Jul 29, 2020 •

edited

jurassic-period Jul 29, 2020

amshkv Jul 29, 2020

amshkv Jul 29, 2020

amshkv Jul 29, 2020

amshkv Jul 29, 2020

jurassic-period Jul 29, 2020

jurassic-period Jul 29, 2020

amshkv Jul 29, 2020

jurassic-period commented Jul 29, 2020

jurassic-period commented Jul 29, 2020

new setting button for CBPlayer #768

new setting button for CBPlayer #768

Conversation

jurassic-period commented Jul 29, 2020

codecov-commenter commented Jul 29, 2020 • edited

Codecov Report

jurassic-period commented Jul 29, 2020

amshkv left a 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

amshkv Jul 29, 2020 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jurassic-period Jul 29, 2020 • edited

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

jurassic-period commented Jul 29, 2020

jurassic-period commented Jul 29, 2020

codecov-commenter commented Jul 29, 2020 •

edited

amshkv Jul 29, 2020 •

edited

jurassic-period Jul 29, 2020 •

edited