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

feat: categories for Icons added #1

Merged
merged 1 commit into from
Jan 10, 2024
Merged

Conversation

kayman233
Copy link
Contributor

@kayman233 kayman233 commented Jan 10, 2024

Добавлена логика для соответствия категории иконки.
Поправлены мелкие ошибки.

@kayman233 kayman233 marked this pull request as ready for review January 10, 2024 08:27
@@ -29,6 +29,15 @@ interface FormProps {
export const Form: FC<FormProps> = ({ onSubmit = () => {}, iconsMetaData }) => {
const [state, setState] = useState<FormPayload>({ ...defaultState, iconsMetaData });

useEffect(() => {
if (iconsMetaData.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

некритично

можно сделать инверсию

Copy link
Contributor Author

Choose a reason for hiding this comment

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

поправил

@@ -24,6 +24,7 @@ export const prettify = (source: string) => prettier.format(source, prettierSett

export const getFilesPath = (iconName?: string, iconSize?: number) => ({
iconSourceExport: 'packages/plasma-icons/src/scalable/index.ts',
iconSourceComponent: 'packages/plasma-icons/src/scalable/Icon.tsx',
Copy link
Contributor

@neretin-trike neretin-trike Jan 10, 2024

Choose a reason for hiding this comment

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

а для чего этот файл тоже коммитить надо при экспорте иконок? 🤔 Он же вроде всегда статичный должен оставаться

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

да, из-за категорий

(octokit: Octokit, owner: string, repo: string) =>
<T>(fn: (...args: any[]) => Promise<T>) =>
(...args: any[]) =>
fn(octokit, owner, repo, ...args);
Copy link
Contributor

Choose a reason for hiding this comment

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

а здесь что-то поменялось? Не могу найти разницу кроме переноса строк

Copy link
Contributor Author

Choose a reason for hiding this comment

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

видимо просто eslint

.trim()
.replace(/^(\s*)[a-zA-Z_]+(\d\d)/g, '')
.replace(/\s/g, '');
return upperFirstLetter(withoutPrefix);
Copy link
Contributor

Choose a reason for hiding this comment

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

можешь пожалуйста добавить в коментах пример строки, которая в итоге разбирается, и что получается из неё? А то я этого в своё время не сделал и сейчас этот код трудно читается

Copy link
Contributor Author

Choose a reason for hiding this comment

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

добавил комменты

const { width, name } = selection;
const { type } = selection;

if (type === 'FRAME') {
Copy link
Contributor

Choose a reason for hiding this comment

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

давай инверсию сделаем? Кажется, что тут можно

Copy link
Contributor Author

Choose a reason for hiding this comment

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

поправил

});

return await Promise.all(
nodes.map(async (node) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

я бы это вынес в какую-нибудь функцию, в которой по названию будет понятно, что тут происходит

Copy link
Contributor Author

Choose a reason for hiding this comment

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

поправил

@@ -60,20 +44,29 @@ export const IconList: FC<IconListProps> = ({ onChangeIconsName, iconsMetaData }
[state, onChangeIconsName],
);

useEffect(() => {
if (iconsMetaData.length > 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

тут тоже

Copy link
Contributor Author

Choose a reason for hiding this comment

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

поправил

@kayman233 kayman233 force-pushed the kayman233/new-export-rules branch from 670929d to 6ed059e Compare January 10, 2024 11:37
@kayman233 kayman233 merged commit 4613331 into master Jan 10, 2024
1 check passed
@kayman233 kayman233 deleted the kayman233/new-export-rules branch January 10, 2024 11:51
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