Skip to content

Simplify amount implementation #1101

Closed
stepancar opened this issue Mar 9, 2020 · 6 comments · Fixed by #1105
Closed

Simplify amount implementation #1101

stepancar opened this issue Mar 9, 2020 · 6 comments · Fixed by #1105
Assignees

Comments

@stepancar
Copy link
Contributor

https://habr.com/ru/post/491120/
@nin-jin
Указал нам на проблемы amount

Предлагает дойти до реализации в таком вот виде

<h3 class="amount">
    <span class="amount__major">1 233</span>
    <span class="amount__minor">,43 ₽</span>
</h3>

Большинство нод в свое время было добавлено из за разности прозрачности отдельных элементов. Думаю щас большинство не актуально.

DIv в span (facepalm)

Давайте попробуем причесать

@stepancar
Copy link
Contributor Author

@nin-jin, не стесняйтесь нам создавать issue, если заметили проблему, а еще лучше PR. В любом случае, Спасибо за нелестную оценку!

@nin-jin
В статье примерчик

const FeeButton = ( { id , submit , size = 'm' , theme = 'dark' , className = '' } ) => (
    <Button
        onClick={ submit }
        className={ "fee-button__submit " + className }
        data-test-id={ id }
        size={ size }
        theme={ theme }
        >
        <Label
            className="fee-button__prefix"
            size={ size }
            theme={ theme }
            >
            { l10n( 'fee-button__prefix' ) }
        </Label>
        <Amount
            className="fee-button__fee"
            data-test-id={ id + '/fee' }
            amount={ fee }
            size={ size }
            theme={ theme }
        />
    </Button>
)

Подскажите, где такой взяли?

@nin-jin
Copy link

nin-jin commented Mar 9, 2020

Нигде, разумеется, просто представил что бы мне пришлось написать, используя я ваши компоненты. Если что-то лишнее - скажите ,я поправлю статью.

@stepancar
Copy link
Contributor Author

@nin-jin
Да не, править в статье ничего не надо)

theme у нас проставляется один раз с помощью ThemeProvider, но так же можно и явно поставить через пропсы.
для чего вам в таком примере className - тоже не понятно.

в ожидаемом примере из статьи

<Button id="pay-submit" onClick={ submit }>
    <Label id="pay-prefix">{ payPrefix }</Label>
    <Amount id="pay-fee" amount={ fee }/>
</Button>

вы их и не используете. А пример выглядит намного минималистичнее.

Плюс, если дальше развивать тему в "Плохом примере" вы описываете

const FeeButton = ( { id , submit , size = 'm' , theme = 'dark' , className = '' } ) => ( ....
А в вашем примере этого нет. Суммарно поэтому ваш пример выглядит много лучше. Ну это ладно)

А вот по поводу size - вы предлагаете брать size от родителя внутри вложенного компонента?

@nin-jin
Copy link

nin-jin commented Mar 10, 2020

theme у нас проставляется один раз с помощью ThemeProvider, но так же можно и явно поставить через пропсы.

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

в ожидаемом примере из статьи вы их и не используете.

Потому, что задание их через пропсы не имеет смысла. Когда один компонент собираешь из других компонент - незачем вручную прокидывать тему во вложенные компоненты.

А в вашем примере этого нет.

Потому что там не реакт вовсе. Обычный html-шаблон.

А вот по поводу size - вы предлагаете брать size от родителя внутри вложенного компонента?

Да, это тоже контекстно зависимая штука. Если я указал size для компонента, то ожидаю, что он увеличится со всем содержимым, а не только паддинги вокруг него.

@stepancar
Copy link
Contributor Author

stepancar commented Mar 10, 2020

theme у нас проставляется один раз с помощью ThemeProvider, но так же можно и явно поставить через пропсы.

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

Не совсем так в вашем случае для темы ничего делать не нужно, она сама прорастет.
(Хотел собрать вам пример в нашей песочница, да как оказалось , у нас песочница сломалась как раз недавно)

А вот по поводу size - вы предлагаете брать size от родителя внутри вложенного компонента?

Да, это тоже контекстно зависимая штука. Если я указал size для компонента, то ожидаю, что он увеличится со всем содержимым, а не только паддинги вокруг него.

Тут я согласен. По имплементации как предлагаете прокидывать size? react контекстом?

@nin-jin

@nin-jin
Copy link

nin-jin commented Mar 10, 2020

Не совсем так в вашем случае для темы ничего делать не нужно, она сама прорастет.
(Хотел собрать вам пример в нашей песочница, да как оказалось , у нас песочница сломалась как раз недавно)

Это вы исходите из того, что на ваших компонентах будут делать лишь приложения, но не другие компоненты. Представьте, каково будет пользователю моих и ваших компонент вперемешку. В одних компонентах тему можно задать через пропсы, в других нельзя.

Тут я согласен. По имплементации как предлагаете прокидывать size? react контекстом?

Ну да, контекстом. Или даже просто через css-zoom.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants