Add UI monorepo package #56
Conversation
There was a problem hiding this comment.
Overall I really like the idea of extracting all current dumb components (that are used in the storybook) to another monorepo package so we can publish it to npm and if needed move it to another repo at some point. However, I don't understand the current structure: you added a few random components and nothing was removed from the old location. I don't see previous components, try to make a bit fewer changes to actual components implementation / naming, otherwise provide the motivation. I'd be more than happy to see old + new components like scoreboard UI (or improvements in current ones).
Overall having this structure is good, but I think its too complicated. The reason I don't really like the current file structure is that it looks like you were inspired by the mui structure which is a bad sign. It now more looks like a company project with a heavy structure & a lot of not-needed requirements imo. If it goes this way we will eventually have generators (just like in angular). But, again, it's a good direction!
I haven't really looked at the component implementation or tested it yet for now...
| @@ -0,0 +1,11 @@ | |||
| { | |||
There was a problem hiding this comment.
my standard prettier config that i use across all projects (also aligns with this project)
| { | |
| { | |
| "semi": false, | |
| "singleQuote": true, | |
| "proseWrap": "never", | |
| "tabWidth": 4, | |
| "trailingComma": "all", | |
| "arrowParens": "avoid", | |
| "printWidth": 160, | |
| "endOfLine": "auto" | |
| } |
There was a problem hiding this comment.
Сможешь ли его вынести в корень проекта - отформатировав его весь после этого, чтобы я мог его активировать у себя и убрать этот?
There was a problem hiding this comment.
i dont really understand what the problem is on your side...
unfortunately, i can't use prettier for the whole project because of its specifics e.g. in some files it makes code less readable
| "files": ["packages/**/*"], | ||
| "rules": { | ||
| "semi": [ | ||
| "error", |
There was a problem hiding this comment.
why add? not clear. half of the project uses it and the other doesn't.
|
|
||
| --ui_font_weight_bold: 900; | ||
|
|
||
| --ui_color_black: #000000; |
There was a problem hiding this comment.
whats all that? why we cant just color and currentColor if needed in other component that seems more logical, the same for font size
There was a problem hiding this comment.
Это реализация базовой возможности изменять темы UI компонентов использую css-переменные.
Например на телефонах, возможно, шрифт нужно сделать покрупнее. Тут мы просто возьмем и поменяем css переменную в файле с темой для телефона, и во всем проекте поменяется размер шрифта.
Если нужен другой размер шрифта для всех платформ - то нужно создать и новый компонент типографики
There was a problem hiding this comment.
Те базовые цвета которые Minecraft реализует у себя внесены отдельно, все компоненты типографики будут иметь возможность перекрашиваться относительно hex который передается внешне
| font-weight: var(--ui_font_weight_bold); | ||
| } | ||
|
|
||
| .rootBlack { |
There was a problem hiding this comment.
jeez, can we simplify these styles somehow? Let's do a smarter approach with some sort of single map here rather than a lot copy-pasting (I mean code duplication). I'll look at it once again later to provide more specific thoughts about that
There was a problem hiding this comment.
Кажется css решения будут всегда производительнее, gzip и brotli хорошо ужимают повторяющиеся части
|
|
||
| import styles from './AppRoot.module.css'; | ||
|
|
||
| export interface AppRootProps extends HTMLAttributes<HTMLDivElement> {} |
There was a problem hiding this comment.
I think it is better to use ComponentType<'div'> for singularity.
For the same reason, I think its better to not export these interfaces and in other files do something like ComponentType<typeof AppRoot> since its a recommended react approach IMO. Overall this would be more logical and easier to understand
| "prettier:check": "prettier --check './**/*.{ts,tsx}'" | ||
| }, | ||
| "peerDependencies": { | ||
| "classnames": "^2.5.1", |
There was a problem hiding this comment.
Базовый проект уже имеет эту зависимость - при обновлении в базовом проекте, можем забыть обновить здесь, тогда будет установлено две зависимости одного пакета с разными версиями - возникнет дубль
| Component?: ElementType; | ||
| } | ||
|
|
||
| export type StylesMap<T extends string> = Record<T, string>; |
| "name": "@prismarine-web-client/ui", | ||
| "version": "1.0.0", | ||
| "main": "./src/index.ts", | ||
| "scripts": { |
There was a problem hiding this comment.
Add private: true for now we're not going to publish it since I don't own @prismarine-web-client
yes, that's what the current dumb ui components do! they are used in storybook, I really liked it when you have base component + provider and they are together. let's see what we can do next and what profit we will have from it! also see my comment above |
|
but again, try to focus on actually needed components rather than copying or using someone else pattern for the common web application, this project is different. For example mui uses |
|
Угу, этот код пока в драфте, я планирую перенести все те компоненты которые уже сделаны и удалить из базовой директории в рамках этого же PR |
|
По поводу структуры директорий: как по мне она очень простая - для каждой сущности своя папка - components, hooks, etc. |
im totally fine with the current component structure I like it when everything is split across modules and the structure is clear, but can we somehow have fewer |
Приводим UI в порядок, все компоненты должны лежать отдельно и не знать ничего про сам клиент - отвечать только за отображние UI