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

Edit achievements #437

Open
wants to merge 22 commits into
base: main
Choose a base branch
from

Conversation

AleksandrKosmylev
Copy link
Contributor

Предыдущий пул-реквест был неполным, отправил не все измененные файлы.(

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

Снимок экрана от 2024-06-15 17-57-59

По-прежнему в репозитории остается код отвечающий за переход на страницу только с общей статикой:
в views, url и в achievements.py. Cейчас для них нет ссылки для перехода.

Если данное такой вариант исполнения подходит, то ненужный код удалю.

@AleksandrKosmylev
Copy link
Contributor Author

AleksandrKosmylev commented Jun 15, 2024

Извиняюсь, за неправильно отправленный pull- request # 4c58a85 "edit_achievements" в него не упаковал все изменения. досылал во втором.

@fey
Copy link
Collaborator

fey commented Jun 18, 2024

а тесты есть на новый роут?


<div class="progress d-flex justify-content-between" style="height: max-content;">
<div class="bg-info text-dark text-start aligns-items-center" role="progressbar" style="width: {{ contribution|div:achievement_made_count|mul:100|floatformat:0 }}%;" aria-valuenow="25" aria-valuemin="0" aria-valuemax="100">
<dl class="p-2 fs-6 mb-0" style="width:50vw;">
Copy link
Collaborator

Choose a reason for hiding this comment

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

тут какой-то скастомный стиль используется. Давайте от такого избавляться (он инлайн стилей). Посмотрите, есть ли в бутстрапе похожий класс, если нет, то можно добавить кастомный класс, который именуется через префикс x-*, возможно есть другие кастомные классы уже, можно на них посмотреть.

Copy link
Collaborator

Choose a reason for hiding this comment

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

А тут не поправили? Посмотрите, как в сикпе сделаны кастомные стили - https://github.com/Hexlet/hexlet-sicp/blob/f02ff453e6c8b045764f497fe536e8ecd24913b6/resources/sass/_custom.scss#L1-L23

Copy link
Contributor Author

Choose a reason for hiding this comment

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

style="height: max-content и style="width:50vw перенесены в base.css как кастомные стили.

style="width: {{ contribution|div:achievement_made_count|mul:100|floatformat:0 }} используемый прогрессбаром остается, так как увидел подобное решение в образце от bootstrap

{% load i18n static mathfilters %}

<div class="progress d-flex justify-content-between" style="height: max-content;">
<div class="bg-info text-dark text-start aligns-items-center" role="progressbar" style="width: {{ contribution|div:achievement_made_count|mul:100|floatformat:0 }}%;" aria-valuenow="25" aria-valuemin="0" aria-valuemax="100">
Copy link
Collaborator

Choose a reason for hiding this comment

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

здес стиль используется чтобы задать прогерсс бар?
Можно ли расчет width засунуть в хелпер-функцию и здесь просто вызывать?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

здес стиль используется чтобы задать прогерсс бар? Можно ли расчет width засунуть в хелпер-функцию и здесь просто вызывать?

  • Да , style="width" отвечает за заполнение полосы прогресса
    https://getbootstrap.com/docs/5.0/components/progress/

  • процедура получения данных для таблицы и сама таблица c личными достижениями сделана по аналогии с achievements.py(было уже разработано раньше), где получали общую статистику.
    Непонятно про функцию хелпер: функция должна быть реализована во view и передана в шаблон, где будет получать аргументы для вычисления процента выполнения?

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.

Единственный вариант реализации функции-хелпера, который придумал через @register.simple_tag :

@register.simple_tag
def calc_percent_achievement(numerator, denominator):
"""Get contributor statistics and required quantity."""
if numerator is not None:
if numerator / denominator > 1:
return 100.0
return numerator / denominator * 100
return 0

размеры кода остаются те же. объем кода при этом решении почти не сократился.

@@ -0,0 +1,18 @@
{% load i18n static mathfilters %}

<div class="progress d-flex justify-content-between" style="height: max-content;">
Copy link
Collaborator

Choose a reason for hiding this comment

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

здесь тоже какой-то кастомный styel

Copy link
Contributor Author

Choose a reason for hiding this comment

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

перенесено в base.css как кастомный стиль.

contributors/urls.py Outdated Show resolved Hide resolved
<li><p class="h6">{% trans "Other projects" %}</p></li>
</div>
<div class="col">
<li> </li>
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
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

@AleksandrKosmylev AleksandrKosmylev Jun 30, 2024

Choose a reason for hiding this comment

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

элемент списка специально выполнен пустым для того чтобы сохранить оформление

@@ -7,7 +7,22 @@
</ul>
</div>
<div class="col">
<p class="h6">{% trans "Other projects" %}</p>
<ul class="list-unstyled my-0">
Copy link
Collaborator

Choose a reason for hiding this comment

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

Вообще тут получилось очень страшно, на будущее его можно переделать.
Списки с дивами не используются. Если у вас список, то между ul > li нет блока. Можете посмотреть, как выглядит футер в код бейзиксе или в самом Хекслете.
image

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.

добавлю новый issue

@@ -0,0 +1,109 @@
from django.db.models import Count, Q, Sum # noqa: WPS235, WPS347
Copy link
Collaborator

Choose a reason for hiding this comment

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

Здесь снова используется игнорирование правил. Вообще от WSP в Хекслете мы отказались ,поэтому и в проекте его можно убрать.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

пока убирал свои noqa увидел,что во многих местах есть строки закомментированные с A,WPS.
Не убирал в тех местах, которые не касаются моего изменения

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.

добавлю новый issue

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Добавил

@fey
Copy link
Collaborator

fey commented Jun 18, 2024

Попробуйте разобраться с кастомными стилями
с настрокой прогресс-бара, попробуйте использовать функцию хелпер.

@AleksandrKosmylev
Copy link
Contributor Author

а тесты есть на новый роут?
В последнем коммите добавлен тест

В коммите a68db2e:

  • добавлен тест на новый роут 'contributor_achievements/slug:slug'
  • изменен размер максимальный размер строки для линтера с 79 до 120 знаков и
    во всем проекте убраны комментарии для сокрытия ошибки (# noqa: E501)
  • внесены новые ошибки в игнор лист flake8

@AleksandrKosmylev
Copy link
Contributor Author

@fey, можешь прокомментировать правки?)

@fey
Copy link
Collaborator

fey commented Jul 4, 2024

если визуально ок, давайте мержить. @sgmdlt
fyi @Malcom1986

@HelenOne
Copy link

HelenOne commented Jul 8, 2024

@sgmdlt

@AleksandrKosmylev
Copy link
Contributor Author

Есть ли какие-то замечания, которые нужно исправить или доделать?

@fey
Copy link
Collaborator

fey commented Jul 17, 2024

тут ждем ревью @sgmdlt , тыкнул его в рабочем чатике)

@sgmdlt
Copy link
Collaborator

sgmdlt commented Jul 18, 2024

Добрый день, а есть ссылка на деплой?

@AleksandrKosmylev
Copy link
Contributor Author

Добрый день, а есть ссылка на деплой?

@sgmld, хотел уточнить, что за ссылка?

@sgmdlt
Copy link
Collaborator

sgmdlt commented Jul 22, 2024

Ага, все хорошо, я развернул локально.

Давайте немного поправим как это выглядит. Для референса я бы взял достижения, например, в стиме.
Screenshot from 2024-07-22 18-08-23

На вкладке "global" видны достижения пользователя и процент достижений среди всех. Достижения всех отсортированы по убыванию от самого частого до самого редкого среди игроков. Если у пользователя нет такого достижения то его иконка отображается серым (это можно как-то переделать на ваше усмотрение). А также число процентов отображается поверх прогресс бара.

Персональные же достижения можно аналогично разделить на выполненные и те, что в прогрессе (верхняя часть) и не выполненные (нижняя). И также как то обозначить не выполненные, в стиме например их иконки сделаны серым.

Screenshot from 2024-07-22 18-15-30

Между собой выполненые и не выполненные можно также сортировать по сложности. Например, если пользователь сделал 1 и 10 пулл-реквестов, то в верхнем блоке будут идти достижения "1", затем "10". А в блоке невыполненных "25", "50", "100"

@AleksandrKosmylev
Copy link
Contributor Author

Оформление по шаблону сделал

@AleksandrKosmylev
Copy link
Contributor Author

@fey, @sgmdlt хотел уточнить по внешнему виду после правок. потом решить проблему "This branch has conflicts that must be resolved" и lвигаться в этому дальше по этой ветке pull request?

@fey
Copy link
Collaborator

fey commented Sep 17, 2024

@AleksandrKosmylev давайте фиксить конфликты. Хотелось бы смержить, двигаться дальше. У меня вроде глобальных замечаний нет. Если что-то не фиксим, то давайте ставить тудушки на будущий фикс.

@AleksandrKosmylev
Copy link
Contributor Author

@fey , готово

@fey fey requested a review from sgmdlt October 7, 2024 14:46
@fey
Copy link
Collaborator

fey commented Oct 7, 2024

@sgmdlt review pls

@AleksandrKosmylev
Copy link
Contributor Author

@sgmdlt review pls

@AleksandrKosmylev
Copy link
Contributor Author

@fey , есть замечания?

@fey
Copy link
Collaborator

fey commented Oct 28, 2024

У меня нет, ждем @sgmdlt пинганул его еще раз в рабочем чяте

comment_ranges_for_achievements = [1, 25, 50, 100, 200]
edition_ranges_for_achievements = [1, 100, 250, 500, 1000]

def get_context_data(self, **kwargs):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Я бы разделил метод на несколько. Сейчас это огромная стена кода. Выделите подметоды, сделайте их приватными (_method_name), разделите логику.

context['pull_requests'] = contributions['contributor_pull_requests']
context['issues'] = contributions['contributor_issues']
context['comments'] = contributions['contributor_comments']
editions = sum([
Copy link
Collaborator

Choose a reason for hiding this comment

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

Для sum() не надо создавать список внутри. Нужно убрать квадратные скобки. И ниже в другом sum тоже

@AleksandrKosmylev
Copy link
Contributor Author

@fey ,
@sgmdlt ,
готово

@fey fey requested a review from sgmdlt November 18, 2024 10:07
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.

4 participants