Tag user List logic by whywhyy · Pull Request #501 · hyochan/hackatalk

whywhyy · 2021-10-10T09:57:55Z

Specify project

Client

Description

Tag user logic

Logic to get a list of users to tag when editing text

tagUserVideo.mov

Related Issues

issue #473

Checklist

Before you create this PR confirms that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the [CLA].
Run yarn lint && yarn tsc
Run yarn test or yarn test -u if you need to update snapshot.
I am willing to follow-up on review comments in a timely manner.

CLAassistant · 2021-10-10T09:58:00Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ hyochan
❌ whywhyy

whywhyy seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

codecov · 2021-10-10T10:04:40Z

Codecov Report

Merging #501 (42c2c19) into master (83152db) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #501   +/-   ##
=======================================
  Coverage   76.09%   76.09%           
=======================================
  Files          64       64           
  Lines        1163     1163           
  Branches      130      130           
=======================================
  Hits          885      885           
  Misses        276      276           
  Partials        2        2

Flag	Coverage Δ
unittests	`76.09% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83152db...42c2c19. Read the comment docs.

geoseong

Hi! 👋
I leave some suggestions from your code. so please check this and let me know what your thinking. thank you!

client/src/components/pages/Message.tsx

client/src/components/uis/GiftedChat.tsx

hyochan · 2021-10-14T15:54:44Z

How does this differ with #505 ?

whywhyy · 2021-10-15T00:59:57Z

How does this differ with #505 ?

#505 is the UI work of the #501 PR.
Currently, #501 tag users are expressed in simple text, but in #505 , the user's profile image is displayed.

Is there a way to merge two different PRs?
Should I write a comment to review the #505 after the #501 is merged?

hyochan · 2021-10-15T02:19:04Z

How does this differ with #505 ?

#505 is the UI work of the #501 PR. Currently, #501 tag users are expressed in simple text, but in #505 , the user's profile image is displayed.

Is there a way to merge two different PRs? Should I write a comment to review the #505 after the #501 is merged?

There are two ways I can tell.

Work on separate prs just now and when merged, one other PR should do rebasing.
Give a PR to the current branch instead of master.

Try out that fits your needs.

hyochan

Thanks for the PR! The idea looks great~!

Hope you to cleanup some of the codebase 🙏

client/src/components/pages/Message.tsx

hyochan · 2021-10-15T02:21:41Z

client/src/components/uis/GiftedChat.tsx

  renderViewMenu: (): React.ReactElement => <View />,
  message: '',
  onChangeMessage: (): void => {},
+  onChangeSelection: (): void => {},


This needs better typings.

props is a function.

Do you have the proper Shared.defaultProps ?

refactor : onChangeSelection -> onChangeCursor

Thanks for the changes but I think we need additional arguments to this typing!

It has been modified as follows. please check.

'_' was added because of an unused lint error.

Shared.defaultProps = { ... onChangeMessage: (_message: string): void => {}, onChangeCursor: (_param: {start: number; end: number}): void => {}, ...

client/src/components/pages/Message.tsx

…ursor

hyochan

May I ask you for two wishlists?

Can we make tag list absolute? It does not look nice to push messages upward when tag list appears.
Can we have user thumbnail?

1,2 are wishlist is most like a slack. It doesn't have to be the same but you can refer to it for understanding.

whywhyy · 2021-10-17T10:20:49Z

May I ask you for two wishlists?

Can we make tag list absolute? It does not look nice to push messages upward when tag list appears.

Can we have user thumbnail?

(with @hankkuu )
Added image. Please check the code.

add

query : add photoURL
ui : uis/TagUserListItem.tsx

hyochan · 2021-10-21T06:51:37Z

@whywhyy Looks like the CLA i not agreed from the user here 324e7ff

hyochan

Could you kindly fix styles a bit to make this PR more complete?

Please make the floating popup close the message input. Currently, it is a bit far away.

Below is how it looks on web.
I think the chatter shouldn't be included in the tag list.

client/src/components/uis/TagUserListItem.tsx

…into feat/tag-users

…e message

whywhyy · 2021-10-22T12:57:20Z

Could you kindly fix styles a bit to make this PR more complete?

Please make the floating popup close the message input. Currently, it is a bit far away.

Below is how it looks on web.

I think the chatter shouldn't be included in the tag list.

thanks for the good point The UI has been modified as follows.

The UI has been modified as follows.

whywhyy · 2021-10-22T13:16:13Z

@whywhyy Looks like the CLA i not agreed from the user here 324e7ff

Something is wrong. I agreed. Agreed with other IDs, but CLA is not passed.
I agree with the cla but I don't agree with the software...
Can I replace this with a message instead of software consent?

hyochan · 2021-10-23T12:01:28Z

@whywhyy Looks like the CLA i not agreed from the user here 324e7ff

Something is wrong. I agreed. Agreed with other IDs, but CLA is not passed. I agree with the cla but I don't agree with the software... Can I replace this with a message instead of software consent?

Got it!

hyochan

Thanks for the great work!

hyochan · 2021-10-27T05:36:03Z

@all-contributors Please add @whywhyy for code

allcontributors · 2021-10-27T05:36:11Z

@hyochan

I've put up a pull request to add @whywhyy! 🎉

hyochan · 2021-10-27T05:40:04Z

@all-contributors please add @hankkuu for code

allcontributors · 2021-10-27T05:40:06Z

@hyochan

@hankkuu already contributed before to code

whywhyy changed the title ~~Tag user logic~~ Tag user user List logic Oct 10, 2021

hyochan added 🍗 enhancement New feature or request 🎯 feature New feature labels Oct 10, 2021

whywhyy changed the title ~~Tag user user List logic~~ Tag user List logic Oct 10, 2021

hyochan requested review from 0916dhkim, JongtaekChoi and geoseong October 10, 2021 10:06

whywhyy added 2 commits October 12, 2021 10:14

feat : 전체 태그 유저

03485a1

fix : tag text proper position

1b216a7

whywhyy force-pushed the feat/tag-users branch from c3a49a4 to 1b216a7 Compare October 12, 2021 01:17

fix : user text match startswith -> includes

fba0931

geoseong suggested changes Oct 12, 2021

View reviewed changes

whywhyy added 3 commits October 13, 2021 22:40

fix : duplicated user type -> def type User

1137c52

fix : duplicated type -> use User type

f79227d

fix : ui func simplify

b22116f

whywhyy requested a review from geoseong October 13, 2021 14:36

hyochan mentioned this pull request Oct 14, 2021

Tag user List logic (add ui) #505

Closed

5 tasks

hyochan suggested changes Oct 15, 2021

View reviewed changes

whywhyy added 4 commits October 15, 2021 16:13

refactor : input function name clearly onChangeSelection -> onChangeC…

174b560

…ursor

refactor: proper nameing cursorFrontText -> cursorPrefix

b941b7d

reafactor : renaming newMessage -> newMessageWithTag

5c3b197

refactor : string type to TextInputKeyPressEventData

862c312

whywhyy requested a review from hyochan October 15, 2021 08:00

hyochan suggested changes Oct 16, 2021

View reviewed changes

fix: message set variable and splice

760d81f

whywhyy added 3 commits October 17, 2021 16:15

fix : defaultProps onChangeCursor add arg type

3b16112

fix : defaultProps add message type

1a590f6

feat : add taged user images

f9692c2

whywhyy requested a review from hyochan October 17, 2021 10:25

refactor : typo seleteTagUser -> selectTagUser

324e7ff

Update snapshots

ffb8277

hyochan force-pushed the feat/tag-users branch from bb9cbce to ffb8277 Compare October 21, 2021 06:54

Update packages and snapshots

66c34ca

hyochan suggested changes Oct 21, 2021

View reviewed changes

client/src/components/uis/TagUserListItem.tsx Outdated Show resolved Hide resolved

client/src/components/uis/TagUserListItem.tsx Show resolved Hide resolved

whywhyy added 3 commits October 22, 2021 21:07

Merge remote-tracking branch 'upstream/master' into feat/tag-users

b90abcb

Merge branch 'feat/tag-users' of https://github.com/whywhyy/hackatalk …

fde5d18

…into feat/tag-users

fix : tagUserListItem style func to number, tag users pop up close th…

42c2c19

…e message

whywhyy requested a review from hyochan October 22, 2021 13:00

hyochan approved these changes Oct 23, 2021

View reviewed changes

hyochan merged commit 6058594 into hyochan:master Oct 23, 2021

allcontributors bot mentioned this pull request Oct 27, 2021

docs: add whywhyy as a contributor for code #509

Merged

Uh oh!

Conversation

whywhyy commented Oct 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Specify project

Description

Related Issues

Checklist

Uh oh!

CLAassistant commented Oct 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

geoseong left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hyochan commented Oct 14, 2021

Uh oh!

whywhyy commented Oct 15, 2021

Uh oh!

hyochan commented Oct 15, 2021

Uh oh!

hyochan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hyochan Oct 15, 2021

Choose a reason for hiding this comment

Uh oh!

whywhyy Oct 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hyochan Oct 16, 2021

Choose a reason for hiding this comment

Uh oh!

whywhyy Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hyochan left a comment

Choose a reason for hiding this comment

Uh oh!

whywhyy commented Oct 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hyochan commented Oct 21, 2021

Uh oh!

hyochan left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

whywhyy commented Oct 22, 2021

Uh oh!

whywhyy commented Oct 22, 2021

Uh oh!

hyochan commented Oct 23, 2021

Uh oh!

hyochan left a comment

Choose a reason for hiding this comment

Uh oh!

hyochan commented Oct 27, 2021

Uh oh!

allcontributors bot commented Oct 27, 2021

Uh oh!

whywhyy commented Oct 10, 2021 •

edited

Loading

CLAassistant commented Oct 10, 2021 •

edited

Loading

codecov bot commented Oct 10, 2021 •

edited

Loading

geoseong left a comment •

edited

Loading

whywhyy Oct 15, 2021 •

edited

Loading

whywhyy Oct 17, 2021 •

edited

Loading

whywhyy commented Oct 17, 2021 •

edited

Loading

hyochan left a comment •

edited

Loading