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

init commit #90

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

init commit #90

wants to merge 1 commit into from

Conversation

sergrage
Copy link

No description provided.

import SortableList from '../../components/sortable-list/index.js';
import NotificationMessage from '../../components/notification/index.js';

export default class Categories {
Copy link
Contributor

Choose a reason for hiding this comment

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

good 👍
лайк за вынесение категорий в отдельный компонент

element; //html element

toggleAccordion = (event) => {
event.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

const subcategoryElementArr = this.element.querySelectorAll("[data-element='subcategoryList']");

subcategoryElementArr.forEach((subcategoryElement, index) => {
const sortableList = new SortableList({ items: subcategoryListArr[index] });
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

loadData = async(start, end) => {
this.element.classList.add("column-chart_loading");
const url = this.url.href + `?from=${start}&to=${end}`;
let data = await fetchJson(url).catch((error) => console.log(error));
Copy link
Contributor

Choose a reason for hiding this comment

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

const data


loadData = async(start, end) => {
this.element.classList.add("column-chart_loading");
const url = this.url.href + `?from=${start}&to=${end}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

не забывайте использовать new URL

} = {}) {
this.formatHeading = formatHeading;
this.range = range;
this.url = new URL(url, BACKEND_URL);
Copy link
Contributor

Choose a reason for hiding this comment

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

good 👍

<div data-element="body" class="column-chart__chart">
${this.getColumnBody(this.data)}
</div>
<div class="${this.chartClasses}" style="--chart-height: 50">
Copy link
Contributor

Choose a reason for hiding this comment

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

что такое 50?

@@ -0,0 +1,176 @@
export default class DoubleSlider {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good


destroy() {
this.remove();
document.removeEventListener('pointermove', this.onThumbPointerMove);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

@@ -1,89 +1,107 @@
const BACKEND_URL = 'https://course-js.javascript.ru';
Copy link
Contributor

Choose a reason for hiding this comment

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

данное значение стоит взять из переменных окружения!

}
createGraph() {
this.data.map(col => {
this.graph += `<div style="--value: ${col.value}" data-tooltip="${col.percent}"></div>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

так делать не следует, не используйте строковую конкатенацию при работе с шаблонами;
лучше соберите массив и приведите его к строке

price: 100,
discount: 0
};
const IMGUR_CLIENT_ID = '28aaa2e823b03b1';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

this.createImagesList();
this.initEventListeners();
onImageListClick = (event) => {
const el = event.target.closest('.imgDeleteBtn');
Copy link
Contributor

Choose a reason for hiding this comment

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

старайтесь именовать переменные как минимум из 3х символов

async load(urlCategories, urlProduct) {
const categories = fetchJson(urlCategories).catch((error) => console.log('loadProduct', error));
const product = fetchJson(urlProduct).catch((error) => console.log('loadProduct', error));
return await Promise.all([categories, product]);
Copy link
Contributor

Choose a reason for hiding this comment

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

пожалуйста воспользуйтесь try...catch для обработки ошибки

const product = this.getFormData();
const result = await fetchJson(`${process.env.BACKEND_URL}api/rest/products`, {
method: 'PATCH',
let product = {
Copy link
Contributor

Choose a reason for hiding this comment

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

const product

let product = {
'images': this.product.images,
'title': escapeHtml(this.subElements.productTitle.value),
'description': escapeHtml(this.subElements.productDescription.value),
Copy link
Contributor

Choose a reason for hiding this comment

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

лайк за использование escapeHtml

return this.categories.map(category => {
if (category.subcategories && category.subcategories.length > 0) {
return category.subcategories.map(subcategory => {
return `<option value="${subcategory.id}">${category.title} &gt; ${subcategory.title}</option>`;
Copy link
Contributor

Choose a reason for hiding this comment

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

обратите внимание на использование new Option

remove() {
this.element.remove();
}
destroy() {
this.remove();
this.subElements.productSave.removeEventListener('click', this.onClick);
Copy link
Contributor

Choose a reason for hiding this comment

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

удалять обработчики с внутренних эл-тов нет необходимости

return `
<div class="rangepicker__day-of-week">
<div>Пн</div>
<div>Вт</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

подумайте как можно сгенерировать данный список динамически

};

return this;
this.subElements.input.removeEventListener('click', this.onInputClick);
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

@@ -1,123 +1,40 @@
import fetchJson from "../../utils/fetch-json.js";
import fetchJson from './../../utils/fetch-json.js';

const BACKEND_URL = 'https://course-js.javascript.ru';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

this.element.classList.remove('sortable-table_loading');

return data;
async loadData(_start, _end, _sort, _order) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

destroy() {
this.remove();
this.subElements = {};
this.subElements.header.removeEventListener('pointerdown', this.sortClick);
window.removeEventListener('scroll', this.infinityScroll);
Copy link
Contributor

Choose a reason for hiding this comment

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

good 👍

element.innerHTML = this.getTemplate();
this.element = element.firstElementChild;
this.subElements = this.getSubElements(this.element);
this.categories = await fetchJson('https://course-js.javascript.ru/api/rest/categories?_sort=weight&_refs=subcategory');
Copy link
Contributor

Choose a reason for hiding this comment

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

не забывайте использовать new URL

url.searchParams.set('from', from.toISOString());
url.searchParams.set('to', to.toISOString());

let sortableTableUrl = `api/dashboard/bestsellers?_start=1&_end=20&from=${from.toISOString()}&to=${to.toISOString()}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

const

}

async update(from, to) {
await this.ordersChart.update(from, to);
Copy link
Contributor

Choose a reason for hiding this comment

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

так делать категорически нельзя!
каждый await блокирует следующий


this.subElements.rangePicker.append(rangePicker.element);

this.subElements.ordersChart.append(this.ordersChart.element);
Copy link
Contributor

Choose a reason for hiding this comment

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

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

<div>
<h1>Edit page</h1>
</div>`;
const URLArray = window.location.href.split('/')
Copy link
Contributor

Choose a reason for hiding this comment

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

так делать не следует, id товара необходимо передать из роутера, обратите внимание на модуль render-page

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.

2 participants