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_separation_into_PR_and_issues_on_the_main_page #261

Merged

Conversation

Tragoedie
Copy link
Contributor

Added split it into two tabs: PR and Issues.
Added buttons: "for the past week", "for the past month"
Added tabs: "PR" and "Issues"

It may be necessary to edit the "IssueInfo" model in order to remove duplication in the file:
contributors/views/home.py? If it needed, is it possible to open a new issue?

@fey
Copy link
Collaborator

fey commented Apr 1, 2023

Deploy please demonstration

@Tragoedie
Copy link
Contributor Author

Deploy please demonstration

What format of demonstration do you need?

@fey
Copy link
Collaborator

fey commented Apr 1, 2023

@Tragoedie railway or render.com with your changes in this PR

@fey
Copy link
Collaborator

fey commented Apr 1, 2023

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

@fey fey requested a review from emp7yhead April 5, 2023 10:34

LATEST_ISSUES_COUNT = 11
LATEST_ISSUES_COUNT = 10
Copy link
Contributor

Choose a reason for hiding this comment

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

Давай переименуем эту константу и используем в get_top10 и в новой функции

Comment on lines 56 to 60
latest_month_issues = Contribution.objects.filter(
repository__is_visible=True,
type__in=['pr', 'iss'],
).order_by('-created_at')[:LATEST_ISSUES_COUNT]
type='iss',
created_at__gte=datetime_month_ago(),
).distinct().order_by('-created_at')[:LATEST_ISSUES_COUNT]
Copy link
Contributor

Choose a reason for hiding this comment

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

Давай вынесем это в отдельную функцию по типу get_top10 и переиспользуем во всех остальных случаях? Будем передавать туда тип объекта - ишью или пр, и за какой период нужны данные.

Comment on lines 1 to 16
function issues_and_pr_grouping(time_group) {
const tops = document.querySelector(`.${time_group}`);
const tabs = tops.querySelectorAll('.nav-link');
tabs.forEach((tab) => tab.addEventListener('click', (e) => {
e.preventDefault();
const activeTab = tops.querySelector('.nav-link.active');
const activeList = tops.querySelector(`.list-group.${activeTab.name}`);
activeTab.classList.remove('active');
activeList.classList.add('d-none');

const newActiveTab = e.target;
const newActiveList = tops.querySelector(`.list-group.${newActiveTab.name}`);
newActiveTab.classList.add('active');
newActiveList.classList.remove('d-none');
}));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Эту функцию можно переиспользовать из top10Contributors.js. Ведь по сути в том файле выполняются те же действия, что и здесь. Зачем увеличивать кодовую базу, когда можно переиспользовать уже готовое решение?

{% include tab with latest_issues=latest_month_pr class='pull-requests' %}
{% include tab with latest_issues=latest_month_issues class='issues' %}
{% endwith %}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

Здесь надо добавить пустую строку в конце

{% include tab with latest_issues=latest_week_pr class='pull-requests' %}
{% include tab with latest_issues=latest_week_issues class='issues' %}
{% endwith %}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

И тут пустая строка нужна

{% trans "for the past month" %}
</a>
</li>
</ul>
Copy link
Contributor

Choose a reason for hiding this comment

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

Еще одна пустая строка

Comment on lines 18 to 34
function issues_time_note_executor() {
const tops = document.querySelector('.latest-issues');
const tabs = tops.querySelectorAll('.issues-and-pr-time-note');
tabs.forEach((tab) => tab.addEventListener('click', (e) => {
e.preventDefault();
const activeTab = tops.querySelector('.issues-and-pr-time-note.active');
const activeList = tops.querySelector(`.latest-issues-and-pr.${activeTab.name}`);
activeTab.classList.remove('active');
activeList.classList.add('d-none');

const newActiveTab = e.target;
const newActiveList = tops.querySelector(`.latest-issues-and-pr.${newActiveTab.name}`);
newActiveTab.classList.add('active');
newActiveList.classList.remove('d-none');
start(newActiveTab.name)
}));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Давай модифицируем функцию time_note() в top10Contributors.js, чтобы она работала еще и с новой таблицей. Добавим параметр, который будет принимать тип фильтруемых данных и в зависимости от этого уже их отображать

Copy link
Contributor

Choose a reason for hiding this comment

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

Тут наверное следует назвать файл не top_issues, а latest_issues_and_prs.

@ssssank
Copy link
Contributor

ssssank commented Apr 20, 2023

@Tragoedie как успехи? Нужна ли какая-нибудь помощь?

@ssssank
Copy link
Contributor

ssssank commented May 3, 2023

@emp7yhead доделаем?

@Tragoedie
Copy link
Contributor Author

@emp7yhead, @ssssank Добрый день, я в данный момент меняю город проживания. Если срочно необходимо доделать, то можете кому-нибудь передать. Я смогу доделать в начале июня.

@emp7yhead
Copy link
Contributor

Я подхвачу, раз такое дело

@emp7yhead emp7yhead force-pushed the Add_separation_into_PR_and_issues_on_the_main_page branch from b91aaa3 to a95df7b Compare May 12, 2023 12:06
@ssssank
Copy link
Contributor

ssssank commented May 12, 2023

Красиво. Сливаем?

@fey
Copy link
Collaborator

fey commented May 12, 2023

а как посмотреть результат? :)
@emp7yhead

@emp7yhead
Copy link
Contributor

я еще не закончил 😉

@emp7yhead emp7yhead force-pushed the Add_separation_into_PR_and_issues_on_the_main_page branch from bc85032 to 761e2fe Compare May 12, 2023 15:25
@ashikov
Copy link
Contributor

ashikov commented May 23, 2023

@emp7yhead чекаво тут? Сливаем?

@ssssank
Copy link
Contributor

ssssank commented May 23, 2023

Я за =D

@ashikov
Copy link
Contributor

ashikov commented May 25, 2023

Сливайте, у меня прав не хватает. :D

@fey fey merged commit e2f8751 into Hexlet:main May 25, 2023
1 check passed
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

5 participants