-
Notifications
You must be signed in to change notification settings - Fork 6
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
Approximate Functional Dependencies frontend #111
base: web-app-dev
Are you sure you want to change the base?
Conversation
title={ | ||
'No clusters have been discovered (functional dependency holds)' | ||
} | ||
description={'Try restarting the task with different parameters'} | ||
icon={<Arrow />} | ||
/> | ||
)} | ||
{!clustersTotalCount && !data.result && ( | ||
<ReportFiller | ||
title={ | ||
'No clusters have been discovered (functional dependency may not hold)' | ||
} | ||
description={'Try restarting the task with different parameters'} | ||
icon={<ArrowCrossed />} | ||
/> |
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.
prop="abc"
web-app/client/src/components/ScrollableNodeTable/implementations/AFD/AFDTable.tsx
Outdated
Show resolved
Hide resolved
.greenHighlighted { | ||
background: $success-25 !important; // stylelint-disable-line declaration-no-important | ||
} | ||
|
||
.greenEven { | ||
background: $success-05 !important; // stylelint-disable-line declaration-no-important | ||
} | ||
|
||
.greenOdd { | ||
background: $success-10 !important; // stylelint-disable-line declaration-no-important | ||
} | ||
|
||
.redHighlighted { | ||
background: $error-25 !important; // stylelint-disable-line declaration-no-important | ||
} | ||
|
||
.redEven { | ||
background: $error-05 !important; // stylelint-disable-line declaration-no-important | ||
} | ||
|
||
.redOdd { | ||
background: $error-10 !important; // stylelint-disable-line declaration-no-important | ||
} |
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.
Зачем important
?
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.
и вроде бы highlighted
не используется
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.
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.
Надо понять, почему так происходит (скорее всего из-за этой строчки).
Может быть можно добавить возможность контролировать альтернацию цветов через пропы ScrollableTable
Почитай ещё про specificity, если ещё не знаешь что это
.checkmark { | ||
path { | ||
fill: $success-100; | ||
stroke: none; | ||
} | ||
} | ||
|
||
.cross { | ||
path { | ||
fill: $error-100; | ||
stroke: none; | ||
} | ||
} |
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.
Это лучше сделать через currentColor
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.
то есть? не понимаю
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.
То есть импортировать иконки с _TEMPLATE_COLOR_
-ом, и менять у них цвет через свойство color
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.
А зачем ты добавила страницу onScroll
?)
Каждый файл, находящиеся в папочке pages
--- это отдельная страница
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.
Плюс этот хук надо ещё использовать в MFD
|
||
useEffect(() => { | ||
shouldIgnoreScrollEvent.current = false; | ||
}, [shouldIgnoreScrollEvent]); |
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.
Не уверен, что по логике тут должна быть эта зависимость. Вообще при изменении рефов компонент не обновляется, поэтому в общем случае писать рефы в зависимостях не стоит. Скорее тут limit
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.
UPD: В оригинале логика такая, что эвент должен игнорироваться пока не подтянутся новые данные. Надо предусмотреть это в API хука
const defaultLimit = 150; | ||
const defaultOffsetDifference = 50; | ||
|
||
export const Scrolling = () => { |
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.
по логике это кастомный хук. А у хуков есть конвеншен по наименованию
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.
И вообще название не очень) Лучше сделать что-то типа useInfiniteScroll
, а initialLimit
и offsetDistance
принимать как параметры
shouldIgnoreScrollEvent.current = false; | ||
}, [shouldIgnoreScrollEvent]); | ||
|
||
return useCallback( |
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.
из кастомных хуков обычно возвращают объект. То есть лучше сделать так:
const onScroll = useCallback(...);
return { onScroll };
web-app/client/src/components/ScrollableNodeTable/implementations/AFD/AFDTable.tsx
Show resolved
Hide resolved
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.
Плюс этот хук надо ещё использовать в MFD
|
||
useEffect(() => { | ||
shouldIgnoreScrollEvent.current = false; | ||
}, [shouldIgnoreScrollEvent]); |
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.
UPD: В оригинале логика такая, что эвент должен игнорироваться пока не подтянутся новые данные. Надо предусмотреть это в API хука
b778af9
to
5ab1d2f
Compare
No description provided.