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

Fix welcome layout #216

Merged
merged 2 commits into from
Apr 14, 2020
Merged

Fix welcome layout #216

merged 2 commits into from
Apr 14, 2020

Conversation

baradusov
Copy link
Contributor

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

Исправляет #214

Демо на heroku.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff            @@
##             master     #216   +/-   ##
=========================================
  Coverage     72.37%   72.37%           
  Complexity      105      105           
=========================================
  Files            39       39           
  Lines           409      409           
=========================================
  Hits            296      296           
  Misses          113      113           

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 8007536...aa51e93. Read the comment docs.

@fey
Copy link
Collaborator

fey commented Apr 14, 2020

🔥 👍

@fey fey merged commit 014a7ce into Hexlet:master Apr 14, 2020
@fey fey requested a review from amshkv April 14, 2020 08:53
@baradusov baradusov deleted the fix-welcome-layout branch April 14, 2020 11:06
<li data-level="{{ $square }}"></li>
@endforeach
</ul>
<div class="d-flex align-items-end flex-column overflow-hidden m-4">
Copy link

Choose a reason for hiding this comment

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

.justify-content-end вместо align-items-end flex-column звучит имхо более разумно
в начале я не мог понять зачем тут колонка и вообще флекс
думаю при правильном классе вопросов будет меньше

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amshkv, почему более разумно, если и то и то решает задачу одинаково?

Copy link

Choose a reason for hiding this comment

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

  1. меньше классов
  2. флексы в основном юзают для главной оси, и колонка из флексов обычно означает что-то для работы именно с колонкой, а не одной строкой
  3. вкусовщина :)

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Поправлю

<div class="col-md-4">
<h2 class="my-3">{{ __('welcome.what_is_here') }}</h2>
<p>{{ __('welcome.about_sicp') }}
<br>
Copy link

Choose a reason for hiding this comment

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

вот это очень плохо, такие вещи решаются через margin/padding, а не через <br>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Сделаю

@endforeach

</ul>
<a class="btn btn-primary" href="{{ (route('my')) }}" role="button">{{ __('layout.welcome.mark_read') }}</a>
Copy link

Choose a reason for hiding this comment

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

если я правильно понимаю, это ссылка, тогда зачем role=button?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@amshkv, ага, это тоже было до. Согласен, что странно сделано.

Copy link
Contributor

Choose a reason for hiding this comment

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

Поправлю

@amshkv
Copy link

amshkv commented Apr 14, 2020

Ну и слишком много кастома от чарта, но его видимо не избежать

@baradusov
Copy link
Contributor Author

@amshkv, я тут только передвигал элементы и пару рядов с колонками обернул. Если найду время, попробую отрефакторить вёрстку.

@econavi econavi mentioned this pull request Apr 19, 2020
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.

5 participants