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

BOX-138 - card list #38

Merged
merged 33 commits into from
Jul 11, 2021
Merged

BOX-138 - card list #38

merged 33 commits into from
Jul 11, 2021

Conversation

OlegBrony
Copy link
Contributor

@OlegBrony OlegBrony commented Jun 30, 2021

in draft:

  1. completed 141 (focus)
  2. completed 140 (title in one line)
  3. adds reaction on search (aka integration?)

adds search ability with trigger on field change, with debounce

ISSUES CLOSED: #61
adds search ability with trigger on field change, with debounce
ISSUES CLOSED: #61
adds search ability with trigger on field change, with debounce
ISSUES CLOSED: #61
babel plugin
export

ISSUES CLOSED: #61
added package from suggestions in review

ISSUES CLOSED: #61
changing view for search page
changes view of users list
changes view of card list

ISSUES CLOSED: #65
changed base button to match design

ISSUES CLOSED: #65
history, reflect (has bugs)

ISSUES CLOSED: #65
# Conflicts:
#	src/app/server.tsx
#	src/entities/card/organisms/card-preview.tsx
#	src/features/search-bar/models/index.ts
#	src/features/search-bar/molecules/search.tsx
#	src/features/search-bar/organisms/search-bar.tsx
#	src/pages/home/index.tsx
#	src/pages/search/model.ts
#	src/pages/search/page.tsx
#	yarn.lock
BREAKING CHANGE:
missed reflect

ISSUES CLOSED: #65
# Conflicts:
#	.gitignore
#	src/pages/search/page.tsx
#	yarn.lock
@OlegBrony OlegBrony marked this pull request as draft June 30, 2021 18:27
# Conflicts:
#	src/features/search-bar/molecules/search.tsx
#	yarn.lock
ellips for title, fix text selection

ISSUES CLOSED: #140
@OlegBrony OlegBrony marked this pull request as ready for review July 4, 2021 17:32
ellips for title, fix text selection
ISSUES CLOSED: #140

ISSUES CLOSED: #91
Copy link
Contributor

@azinit azinit left a comment

Choose a reason for hiding this comment

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

Чет оч много всего намешал в один PR - отсюда и много правок))

Предлагаю все же изолированно сделать еще один PR касаемо CardHover и CardTitle, а здесь потом останутся только изменения связанные с search-логикой

Ну и еще оч много встретил логики в коде связанной с фокусом / логикой перемещения курсора и проч - тож надеюсь дашь коменты)

global.ignores.css Outdated Show resolved Hide resolved
src/api/mock/fixtures.ts Show resolved Hide resolved
src/api/mock/index.ts Outdated Show resolved Hide resolved
src/api/mock/index.ts Show resolved Hide resolved
src/api/mock/index.ts Outdated Show resolved Hide resolved
src/features/search-bar/models/index.ts Outdated Show resolved Hide resolved
src/features/search-bar/models/index.ts Outdated Show resolved Hide resolved
src/entities/user/organisms/user-preview.tsx Outdated Show resolved Hide resolved
src/entities/navigation/index.ts Show resolved Hide resolved
src/entities/card/organisms/card-preview.tsx Outdated Show resolved Hide resolved
@azinit
Copy link
Contributor

azinit commented Jul 5, 2021

@OlegBrony ну и это тож как бы)

image

ellips for title, fix text selection
ISSUES CLOSED: #140
ISSUES CLOSED: #91
@azinit
Copy link
Contributor

azinit commented Jul 6, 2021

@OlegBrony спасиб что внес правки, хорошая работа)
Но как и писал - пока слишком много разнородных изменений в одном PR))

Ну т.е. я бы и не был прям сильно против, если бы ты оставил все остальные правки, но если бы ты убрал в другой PR или модуль работу с навигацией/фокусом - то можно было бы уже вливать хоть щас 🤷‍♂️

image

risen228
risen228 previously approved these changes Jul 8, 2021
Copy link
Contributor

@azinit azinit left a comment

Choose a reason for hiding this comment

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

Не буду пока явно апрувить/реквестить изменения

Но чет все равно часть вещей уже согласованных - все равно здесь не поправлена)

Дай пож знать, мб ты планировал эти вещи позже сделать, я не оч помню на чем мы там в чате сошлись

@@ -0,0 +1,14 @@
:root {
Copy link
Contributor

Choose a reason for hiding this comment

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

Потом бы в vars.css или variables.css переименовал, но некритично)

Comment on lines 28 to +33
/**
* @remark May be in future - make sense to split independent components - CardItem, CardDetails
* @default "item"
* @default "preview"
*/
type?: CardType;
focusItemChanged?: (direction: 'next' | 'prev') => void;
Copy link
Contributor

Choose a reason for hiding this comment

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

Чтоб ты понимал, с чем обычно ассоциируется "preview"))

Го обратно вернем пож? (на item или синонимичное)

image

src/entities/card/organisms/card-preview.tsx Outdated Show resolved Hide resolved
src/features/search-bar/models/index.ts Outdated Show resolved Hide resolved
Comment on lines 3 to 8
// todo:
// currently used only in card-list,
// need to optimize this hook for navigation with keyboard
// e.g. - handle arrow down (focus on cur item index + row len)
export function useFocus() {
const containerRef = useRef<null | HTMLDivElement>(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Дай пож описание тогда, что делает этот хук)

@@ -46,7 +46,7 @@ export const HomePage = () => {
<Main>
<CardList
cards={cards}
getHref={(card) => paths.card(card.id)}
getHref={(card) => paths.card({ id: card.id })}
Copy link
Contributor

Choose a reason for hiding this comment

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

@antonmazhuto вродь не против был

Поэтому давай обратно пож вернем)

image

tsconfig.json Show resolved Hide resolved
Copy link
Contributor

@azinit azinit left a comment

Choose a reason for hiding this comment

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

Пох, давай зальем как есть
Потом как у кого наберется сил - поправим)

А то никогда так не вольем

@azinit azinit merged commit 5942f87 into master Jul 11, 2021
@azinit azinit deleted the feat/138-search-fixes-card-list branch July 11, 2021 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants