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 pages and more #95

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

Conversation

Cat-Malum
Copy link

Не смог разобраться с notification

@@ -0,0 +1,178 @@
export default class DoubleSlider {
element;
Copy link
Contributor

Choose a reason for hiding this comment

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

нужно что-то сделать с форматированием кода, для JavaScript обычно принято использовать 1 таб в размере 2х пробелов

@@ -0,0 +1,66 @@
export default class NotificationMessage {
static activeNotification;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

duration = 2000,
type = 'success'
} = {}) {
this.string = string;
Copy link
Contributor

Choose a reason for hiding this comment

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

лучше назвать это поле message

this.element = wrapper.firstElementChild;
}

show(target = document.body) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

this.remove();
}
element;
draggingElem;
Copy link
Contributor

Choose a reason for hiding this comment

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

нельзя в проекте смешивать отступы табуляций и пробелов!
либо то либо то!

}

moveDraggingAt(clientX, clientY) {
// this.draggingElem.style.left = `${clientX - this.pointerShift.x}px`;
Copy link
Contributor

Choose a reason for hiding this comment

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

не оставляйте закоменченый код

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

const BACKEND_URL = 'https://course-js.javascript.ru';
const BACKEND_URL = process.env.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>`
).join('');
return data.map(item => {
if (window.location.pathname === '/sales') {
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 please don't do this
так делать категорически нельзя! нельзя просто так взять и обратится из компонента к window.location

Copy link
Contributor

Choose a reason for hiding this comment

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

подумайте, может стоит передать какой-то параметр для определения что использовать div или a тег


const toggleSidebar = document.querySelector('.sidebar__toggler');

toggleSidebar.addEventListener('click', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good


for (const page of pages) {
if (pageName.includes(page.dataset.page)) {
page.parentElement.classList.add('active');
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 please don't do this
так делать нельзя! нельзя обращаться к parentElement
найдите данный эл-т с помощью селектора

export default class Page {
element;
components = {};
url = new URL('api/rest/categories', 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

}

destroy() {
for (const component of Object.values(this.components)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

@@ -41,7 +43,8 @@ export default class Page {

async initComponents () {
const to = new Date();
const from = new Date(to.getTime() - (30 * 24 * 60 * 60 * 1000));
const from = new Date(to.getTime() - (30 * 30 * 60 * 60 * 1000));
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 please don't do this
так делать нельзя! подумайте как взять временной диапазон в 1 месяц (в месяце бывает 30 дней, 31, 28 и даже 29)

@@ -73,45 +76,40 @@ export default class Page {
value: customersData.reduce((accum, item) => accum + item),
});

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

components = {};

constructor(productId = '') {
this.productId = productId;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 good

}

async initComponents(productId = '') {
const productData = await new ProductForm(productId);
Copy link
Contributor

@dosandk dosandk Mar 1, 2023

Choose a reason for hiding this comment

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

🔴 please don't do this
так работать не будет await new ProductForm(productId), так делать не стоит
не бывает асинхронных конструкторов

}

renderComponents() {
const topPanel = this.element.querySelector('.content__top-panel');
Copy link
Contributor

Choose a reason for hiding this comment

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

было бы неплохо делать рендер компонент динамически - через цикл

const page = new Page();
let page;

if (path === 'products/edit') {
Copy link
Contributor

Choose a reason for hiding this comment

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

🔴 please don't do this
так делать не следует
передавайте match во все страницы, просто обрабатывайте этот аргумент только в тех страницах где это нужно!

Copy link
Contributor

@dosandk dosandk left a comment

Choose a reason for hiding this comment

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

👍 good
нужно поправить форматирование кода! отступы

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