-
Notifications
You must be signed in to change notification settings - Fork 217
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
js-20221021_maxim-b #84
base: master
Are you sure you want to change the base?
Conversation
src/components/column-chart/index.js
Outdated
@@ -1,74 +1,94 @@ | |||
import fetchJson from './utils/fetch-json.js'; | |||
|
|||
const BACKEND_URL = 'https://course-js.javascript.ru'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
это значение следует взять из переменных окружения: process.env
this.label = label; | ||
this.link = link; | ||
this.value = value; | ||
this.formatHeading = formatHeading; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👍
@@ -0,0 +1,56 @@ | |||
// same as fetch, but throws FetchError in case of errors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ф-ции fetchJson
и escapeHTML
следует вынести на уровень всех компонент и не дублировать
@@ -0,0 +1,66 @@ | |||
export default class NotificationMessage { | |||
static activeNotification; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
лайк за статическое св-во
this.element = element.firstElementChild; | ||
} | ||
|
||
show(parent = document.body) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
src/components/product-form/index.js
Outdated
import escapeHtml from './utils/escape-html.js'; | ||
import fetchJson from './utils/fetch-json.js'; | ||
|
||
const IMGUR_CLIENT_ID = '28aaa2e823b03b1'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
значения следует взять из переменных окружения
@@ -0,0 +1,6 @@ | |||
export default string => string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не дублируйте данные ф-ции в компонентах, вынесите на уровень выше
remove() { | ||
if (this.element) { | ||
this.element.remove(); | ||
} | ||
} | ||
|
||
destroy() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
export default class Page { | ||
element; | ||
subElements = {}; | ||
components = {}; | ||
url = new URL('api/dashboard/bestsellers', BACKEND_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
this.remove(); | ||
this.subElements = {}; | ||
this.element = null; | ||
|
||
for (const component of Object.values(this.components)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
src/pages/sales/index.js
Outdated
|
||
import fetchJson from '../../utils/fetch-json.js'; | ||
|
||
const BACKEND_URL = 'https://course-js.javascript.ru/'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
переменные окружения!
return fetchJson(this.url); | ||
} | ||
|
||
initComponents () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
}); | ||
|
||
const sortableTable = new SortableTable(header, { | ||
url: `api/rest/orders?_start=1&_end=30&createdAt_gte=${from.toISOString()}&createdAt_lte=${to.toISOString()}`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не забывайте про new URL
this.subElements = {}; | ||
this.element = null; | ||
|
||
for (const component of Object.values(this.components)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не все страницы!
не вижу страниц:
- categories
- products
- error 404
|
||
const BACKEND_URL = 'https://course-js.javascript.ru'; | ||
const BACKEND_URL = process.env.BACKEND_URL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👍,
|
||
const IMGUR_CLIENT_ID = '28aaa2e823b03b1'; | ||
const BACKEND_URL = 'https://course-js.javascript.ru'; | ||
const IMGUR_CLIENT_ID = process.env.IMGUR_CLIENT_ID; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👍
import {getSubElements} from '../../utils/helpers'; | ||
import NotificationMessage from '../../components/notification'; | ||
|
||
const BACKEND_URL = process.env.BACKEND_URL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👍
@@ -0,0 +1,139 @@ | |||
import SortableList from '../../components/sortable-list'; | |||
import fetchJson from '../../utils/fetch-json'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👍
wrapper.innerHTML = this.template; | ||
this.element = wrapper.firstChild; | ||
this.subElements = getSubElements(this.element) | ||
this.data = await this.getCategoryData(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👍
} | ||
|
||
getCategories() { | ||
for (const category of this.data) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👍
return this.getSubcategoryItem(subcategory); | ||
}); | ||
|
||
const sortableList = new SortableList({items}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good 👍
async updateCategoryOrder(data) { | ||
const subcategories = []; | ||
const items = data.querySelectorAll('li'); | ||
items.forEach((item, index) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
может лучше воспользоваться map
?
}); | ||
}); | ||
|
||
const url = new URL('api/rest/subcategories', BACKEND_URL); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
это значение лучше объявить сразу в конструкторе
this.showNotification() | ||
} | ||
|
||
showNotification() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
|
||
const BACKEND_URL = 'https://course-js.javascript.ru/'; | ||
const BACKEND_URL = process.env.BACKEND_URL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
|
||
constructor (productId) { | ||
const pathChunks = document.URL.split("/"); | ||
productId = pathChunks[pathChunks.length-1] === "add" ? "add" : pathChunks[pathChunks.length-1]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
не следует мутировать аргументы ф-ции
|
||
destroy() { | ||
this.remove(); | ||
Object.keys(this.components).forEach(componentName => this.components[componentName].destroy()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 good
No description provided.