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

add view completed exercises list #385

Merged
merged 13 commits into from Jul 7, 2020

Conversation

AlexeyShobanov
Copy link
Contributor

Посмотри, пожалуйста, я правильно понял, как это должно выглядеть

@AlexeyShobanov
Copy link
Contributor Author

@fey
Copy link
Collaborator

fey commented Jun 30, 2020

Ага, только можно показывать еще названия упражнений, а глав думаю необязательно
Т.е. либо первым идет название главы вторым (просто номер, например)
Либо группировтаь по номерам групп.

@fey
Copy link
Collaborator

fey commented Jun 30, 2020

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

@fey
Copy link
Collaborator

fey commented Jun 30, 2020

Т.е. можно например показывать все упражнения и на этой вкладке их отмечать/добавлять решение, а с первой убрать. Но как-то подумать щас, чтобы можно было минимальную версию сделать и потом допиливать

@fey fey requested a review from amshkv June 30, 2020 09:16
resources/sass/app.scss Outdated Show resolved Hide resolved
Copy link

@amshkv amshkv left a comment

Choose a reason for hiding this comment

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

Скорее всего часть изменений сделана не тобой, а просто копипаста из-за переноса, но мы руководствуемся принципами бойскаутов и оставляем место после себя чище

resources/sass/app.scss Outdated Show resolved Hide resolved
resources/views/my/index.blade.php Outdated Show resolved Hide resolved
Comment on lines 2 to 3
<div class="d-flex border border-top-0">
<div class="col-12 col-md-4 my-4 border-right">
Copy link

Choose a reason for hiding this comment

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

когда у нас потомки .col - родителем должен быть .row
в твоем случае ты потерял адаптив этими флексами

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

Comment on lines 18 to 19
<div class="col-12 col-md-8 mt-2">
<div class="pl-2 pr-3">
Copy link

Choose a reason for hiding this comment

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

накладываешь отступ на отступ
проверь мобильный вид 320 пикселей, он тебя удивит

<div class="tab-content">
@foreach($mainChapters as $mainChapter)
<div
class="tab-pane card-body {{ $mainChapter->path === '1' ? 'active' : '' }}"
Copy link

Choose a reason for hiding this comment

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

.card-body есть, а где родитель с .card? или я что-то пропустил?

@include('partials.chapter_form_element', ['chapter' => $mainChapter])
</div>
@endforeach
<div class="form-group text-right">
Copy link

Choose a reason for hiding this comment

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

а нужен ли тут .form-group если элемент один?

resources/views/my/progresses/_my_exercises.blade.php Outdated Show resolved Hide resolved
Comment on lines 4 to 7
<div class="d-flex justify-content-between mb-4">
<div class="h3">{{ __('layout.nav.my_progress') }}</h3></div>
<div class="h5">
<a href="{{ route('users.show', $user) }}">{{ $user->name }}</a>
Copy link

Choose a reason for hiding this comment

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

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

@AlexeyShobanov
Copy link
Contributor Author

AlexeyShobanov commented Jun 30, 2020 via email

@amshkv
Copy link

amshkv commented Jun 30, 2020

это всё равно плохое решение, даже с Id
сейчас так добавив тут, это потом начнет расходится по другим страницам и в один момент это станет мешаниной

тут, к сожалению, работает теория разбитых окон, и мы не должны увеличивать энтропию :)

@AlexeyShobanov
Copy link
Contributor Author

Сделал в первом приближении
Посмотреть можно здесь:
https://hexlet-sicp-test.herokuapp.com
Но есть одна серьезная проблема: нужен JS скрипт, чтобы прописывать tab в адресную строку
Иначе при обновлении или перезагрузке постоянно вываливаешься на первый tab
Можно сделать костыль, поменяв табы местами, но это не очень хорошо

@fey
Copy link
Collaborator

fey commented Jul 2, 2020

Смотри, в общем списке показываются решения по отдельности.
Т.е. на каждое решение своя запись.
И получается, что если у нас 2 решения, то запись как бы "дублируется", потому что если мы заходим в детали то по факту всегда сперва видим v1

@fey
Copy link
Collaborator

fey commented Jul 2, 2020

Давай немного причешем то, что есть, А потом уже будем дальше допиливать юзабилити.

@AlexeyShobanov
Copy link
Contributor Author

AlexeyShobanov commented Jul 2, 2020 via email

@fey
Copy link
Collaborator

fey commented Jul 2, 2020

Да, в sqlite свои особенности.

@AlexeyShobanov
Copy link
Contributor Author

исправил:
https://hexlet-sicp-test.herokuapp.com/

@fey
Copy link
Collaborator

fey commented Jul 2, 2020

Давай пока вкладку переименуем в "My Solutions" потому что она именно это показывает.
Уберем ссылку на упражнение, оставим только на решение.
Чтобы не путать пользователя. Обычно мы по привычке жмем по тому, что видим, а видим ссылку под названием упражнения. И попадаем не на решение, а на страничку упражнения.
Но думаю не обязательно так делать, но надо подумать об этом.

@fey fey requested a review from amshkv July 2, 2020 16:06
Copy link

@amshkv amshkv left a comment

Choose a reason for hiding this comment

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

замечания есть, надо их проработать и проведем ревью дальше

resources/sass/app.scss Outdated Show resolved Hide resolved
resources/views/my/index.blade.php Outdated Show resolved Hide resolved
resources/views/my/progresses/_my_chapters.blade.php Outdated Show resolved Hide resolved
resources/views/my/progresses/_my_chapters.blade.php Outdated Show resolved Hide resolved
resources/views/solution/show.blade.php Outdated Show resolved Hide resolved
resources/views/solution/show.blade.php Outdated Show resolved Hide resolved
resources/views/solution/show.blade.php Outdated Show resolved Hide resolved
resources/js/app.js Outdated Show resolved Hide resolved
@@ -1,5 +1,7 @@
// Body
$body-bg: #f8fafc;
//
$nav-tabs-link-active-bg: #fff;
Copy link
Collaborator

Choose a reason for hiding this comment

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

а зачем цвет менять?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Здесь пришлось прописывать цвет таба, так как был изменен цвет основного фона для этого проекта, а он тянет за собой изменение цвета таба (по умолчанию он был белым, как и нужно). Обсуждал это с Андреем, он подсказал, что это решается так

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Как это выглядит можно посмотреть здесь:
https://hexlet-sicp-test.herokuapp.com/

Copy link

@amshkv amshkv left a comment

Choose a reason for hiding this comment

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

кажется всё ок, но может еще что найду :D

newUrl = url.split("#")[0] + hash;
history.replaceState(null, null, newUrl);
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

Может этот код завернуть в функцию, Которую вызывать + убрать var и заменить на const? Просто и правда выглядит, что какой-то чувак по-быстрому накидал решение не разбираясь в тонкостях js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, подумаю как это сделать

Copy link
Collaborator

Choose a reason for hiding this comment

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

Смотри, если с js не хочешь морочиться, то можно и не делать, потом поправим наверное.
Просто из-за var будет глобальная переменная. Ну и вообще тут код не очень написан, как мне кажется.

@fey fey merged commit feaccfc into Hexlet:master Jul 7, 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.

None yet

3 participants