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

updated index page #68

Merged
merged 7 commits into from Jun 24, 2021
Merged

updated index page #68

merged 7 commits into from Jun 24, 2021

Conversation

Surtt
Copy link
Contributor

@Surtt Surtt commented Jun 13, 2021

#63

@fey fey requested a review from amshkv June 13, 2021 17:26
@amshkv
Copy link
Contributor

amshkv commented Jun 15, 2021

а 78-105 что отступы не поправил в index? :)

Copy link
Contributor

@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.

Так, ты большой молодец, что взялся за это дело.
Предвижу тут миллион рабочих задачек, давай как-то делать это итерациями.
Для начала давай поправим что я написал, а дальше будем разбираться)

index.html Outdated Show resolved Hide resolved
m=s.getElementsByTagName(o)[0];a.async=1;a.src=g;m.parentNode.insertBefore(a,m)
})(window,document,'script','//www.google-analytics.com/analytics.js','ga');
<!-- Placed at the end of the document so the pages load faster -->
<script src="https://ajax.googleapis.com/ajax/libs/jquery/1.7.2/jquery.min.js"></script>
Copy link
Contributor

Choose a reason for hiding this comment

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

насколько я знаю, в 5м бутстрапе не нужен jquery, а тут мы его подключаем, ты уверен что оно нам надо? (тот же вопрос к popper.js)

index.html Outdated Show resolved Hide resolved
index.html Outdated
aria-controls="navbarSupportedContent" aria-expanded="false" aria-label="Toggle navigation">
<span class="navbar-toggler-icon"></span>
</button>
<a class="navbar-brand" href="http://www.restapitutorial.ru">Руководство по REST API</a>
Copy link
Contributor

Choose a reason for hiding this comment

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

. это разве не достаточно изменить на корень сайта? href="http://www.restapitutorial.ru" на href=/

Copy link
Contributor

Choose a reason for hiding this comment

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

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

index.html Outdated Show resolved Hide resolved
@Surtt
Copy link
Contributor Author

Surtt commented Jun 15, 2021

а 78-105 что отступы не поправил в index? :)

ты имеешь в виду закоментированный код? Удаляю его? :)

@amshkv
Copy link
Contributor

amshkv commented Jun 15, 2021

ты имеешь в виду закоментированный код? Удаляю его? :)

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

@Surtt
Copy link
Contributor Author

Surtt commented Jun 15, 2021

ты имеешь в виду закоментированный код? Удаляю его? :)
удалять пока наверное не стоит)
смотрю ты отступы везде перекроил) а тут "старые отступы остались"
ну это так, мелкая придирка конечно же

а, ну так это закоментировано, я это даже не смотрел.

@fey
Copy link
Collaborator

fey commented Jun 23, 2021

@amshkv если нет критичных моментв - смержу?

Copy link
Contributor

@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.

Смотри, давай удалим везде этот респонсив стил, поправим нав меню и немного шороху в футере наведем, прям чуть чуть, и в прод!)

index.html Outdated
<!-- Le styles -->
<link href="https://cdn.jsdelivr.net/npm/bootstrap@5.0.1/dist/css/bootstrap.min.css" rel="stylesheet"
integrity="sha384-+0n0xVW2eSR5OomGNYDnhzAbDsOXxcvSN1TPprVMTNDbiYZCxYbOOl7+AMvyTG2x" crossorigin="anonymous">
<link href="https://d7im4lln3lvbg.cloudfront.net/bootstrap/2.0.1/css/bootstrap-responsive.min.css" rel="stylesheet">
Copy link
Contributor

Choose a reason for hiding this comment

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

вот этот хитрый css старый ломает тебе всю верстку с нав меню

Copy link
Contributor Author

Choose a reason for hiding this comment

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

немного шороху в футере наведем, прям чуть чуть

не уверен, что немного получилось :)

index.html Outdated Show resolved Hide resolved
index.html Outdated
</footer>
</div> <!-- /container -->
<hr>
<footer>
Copy link
Contributor

Choose a reason for hiding this comment

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

вот тут как-будто много стало и высоко, может до small сократим?
а еще лучше если отзывы каждого

подрежем, чтобы это таким полотном не было, а красоту потом наводить будем

Copy link
Contributor Author

Choose a reason for hiding this comment

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

и еще, я бы посоветовал бренд имя вынести перед кнопкой-тогглером

Если бренд вынести перед кнопкой, тогда на мобилке бейджик гитхаб перекрывает меню (тоггл)

вот тут как-будто много стало и высоко, может до small сократим?
а еще лучше если отзывы каждого
подрежем, чтобы это таким полотном не было, а красоту потом наводить будем

Это совсем не понял. Какие отзывы?

<!-- Le styles -->
<link href="https://cdn.jsdelivr.net/npm/bootstrap@5.0.1/dist/css/bootstrap.min.css" rel="stylesheet"
integrity="sha384-+0n0xVW2eSR5OomGNYDnhzAbDsOXxcvSN1TPprVMTNDbiYZCxYbOOl7+AMvyTG2x" crossorigin="anonymous">
<link href="https://d7im4lln3lvbg.cloudfront.net/bootstrap/2.0.1/css/bootstrap-responsive.min.css" rel="stylesheet">
Copy link
Contributor

Choose a reason for hiding this comment

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

то же самое

index.html Outdated
</ul>
</div><!--/.nav-collapse -->
</div> <!-- /container -->
<footer class="bg-dark text-light p-4 mt-5">
Copy link
Contributor

Choose a reason for hiding this comment

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

p-4 лишнее, лучше py-4, иначе текст криво выровнен будет

@amshkv amshkv merged commit 18736ce into Hexlet:master Jun 24, 2021
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