Comment on #2 Create a guide/pipeline for coming new categories

Refer to Readme here: <https://github.com/cofacts/rumors-ai-bert/tree/master/GPU_host|https://github.com/cofacts/rumors-ai-bert/tree/master/GPU_host>

#245 Add user agreement files

• Add `LEGAL.md` • Update README to explain the difference between `LEGAL.md` and `LICENSE` • Upgrade copyright holder for MIT license

Comment on #245 Add user agreement files

<https://coveralls.io/builds/36780833|Coverage Status> Coverage decreased (-0.05%) to 86.038% when pulling *<https://github.com/cofacts/rumors-api/commit/b41e0993e7645aa1372fc171b009801f09208c4e|b41e099> on legal* into *<https://github.com/cofacts/rumors-api/commit/c2ff8f2c8f7221bf431fb760438d09b01a8e6608|c2ff8f2> on master*.

mrorz 2021-02-02 14:32:34

markdown 檔居然也算在 coverage 的分母嗎
傻眼

nonumpa 2021-02-02 15:55:04

Coverage status 表示 `src/util/pseudonymDict.js` 下降到 ˙ 75%
應該是 random 造成的吧？

Review on #245 Add user agreement files

#372 Bump urijs from 1.19.2 to 1.19.5

#380 Bump immer from 5.0.0 to 8.0.1

• 1

Review on #245 Add user agreement files

OptimalBits/bull

Premium Queue package for handling distributed jobs and messages in NodeJS.

LINE ENGINEERING

轉移你的 LIFF: 從 Replace 到 Concatenate 模式(持續更新) - LINE ENGINEERING

於 1/18 釋出了一篇新聞 Reminder: On March 1, 2021, "Replace (Backward compatibility mode)" will be removed from the permanent link redirection settings for LIFF app and LINE MINI App，在這個新聞中提到將會在 3/1 移除 LIFF 的 Replace 模式:

• 1

Review on #238 Handle group messages

I'm still reviewing, just send out 2 comments here

Comment on #238 Handle group messages

Please add a paragraph regarding custom dimension setup in GA in the README

Comment on #238 Handle group messages

We can directly Suggested change

#387 Add user aggrement files

• Add `LEGAL.md` • Update README to explain the difference between `LEGAL.md` and `LICENSE` • Upgrade copyright holder for MIT license

#20 Remove git lfs &amp; files

Opendata download keep hitting LFS free quotas, thus remove and relocate files to other places

#17 Move generated zip files to git-lfs instead

Comment on #17 Move generated zip files to git-lfs instead

We are moving away from git-lfs now, see <https://github.com/cofacts/opendata/pull/20|#20>

Review on #387 Add user aggrement files

#21 Add LEGAL.md and update README

• Add LEGAL.md • Update paragraphs regarding terms to latest description

#240 Add user agreement files

• Add `LEGAL.md` • Update README to explain the difference between `LEGAL.md` and `LICENSE` • Upgrade copyright holder for MIT license

Review on #21 Add LEGAL.md and update README for new data access instructions

Review on #20 Remove git lfs &amp; files

Review on #240 Add user agreement files

Comment on #240 Add user agreement files

*Pull Request Test Coverage Report for <https://coveralls.io/builds/37053133|Build 1259>* • *0* of *0* changed or added relevant lines in *0* files are covered. • No unchanged relevant lines lost coverage. • Overall coverage remained the same at *86.42%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*

#388 Implement tutorial page

UI: <https://www.figma.com/file/zpD45j8nqDB2XfA6m2QskO/Cofacts-website?node-id=2011%3A0|https://www.figma.com/file/zpD45j8nqDB2XfA6m2QskO/Cofacts-website?node-id=2011%3A0> Spec: <https://g0v.hackmd.io/sB_zayWjTo-W0R7xe_U34w#%E5%88%86%E9%A0%85%E8%A7%A3%E8%AA%AA|https://g0v.hackmd.io/sB_zayWjTo-W0R7xe_U34w#%E5%88%86%E9%A0%85%E8%A7%A3%E8%AA%AA>

Comment on #388 Implement tutorial page

Just curious: Does eslint-plugin-prettier recognize this file?

#240 Add user agreement files

#387 Add user agreement files

#20 Remove git lfs &amp; files

#21 Add LEGAL.md and update README for new data access instructions

Review on #238 Handle group messages

Thanks for the implementation! I have added some comments and some hints on GA. Happy Lunar New Year!

Comment on #238 Handle group messages

nit: move the definition of `validCategories` outside of `processText()` and change name to `VALID_CATEGORIES` to indicate that it is a constant

Comment on #238 Handle group messages

TODO: remove `req` as discussed in slack.

Comment on #238 Handle group messages

Actually replyCount is identical to number of normal article replies. In `processText()`, we are only querying article replies in the GraphQL, thus `articleReplies.length` is actually identical to `replyCount` and `replyCount` is not needed if we are passing the whole `articleReplies` array in.

Comment on #238 Handle group messages

nit: can move `introKeywords` to oustide of `processText()` and use the name `INTRO_KEYWORDS` to indicate that it's a const

Comment on #238 Handle group messages

I think we should send GA event `Article` / `Selected` / `&lt;selected article id&gt;` if an "identical doc" is found, even if the message is not in selected categories or has no replies. This way we can detect each message's popularity better.

Comment on #238 Handle group messages

If there is a valid `articleReply`, we can send GA event `Reply` / `Selected` / `&lt;selected reply id&gt;` as well.

Comment on #238 Handle group messages

Please add a paragraph regarding custom dimension setup in GA in the README. Actually, since the number of group member is meant to be added together rather than use as a category, we should use <https://support.google.com/analytics/answer/2709828?hl=en|custom metrics> instead of custom dimensions.

Comment on #238 Handle group messages

I am confused by the 4 jobs and the changing `processGroupEvent` mock. May I know what would happen to the 4 jobs here?

Comment on #238 Handle group messages

It seems that it should happen in real case as well. When a job is added to `expiredJobQueue` in `processJob`, it must have an expired timestamp. By the time the job is executed by `expiredJobQueue`, `processGroupEvent` will be invoked, and `isEventExpired()` inside will still throw a `TimeoutError`.

Comment on #238 Handle group messages

Let's use logical or assignment instead of bitwise or assignment Suggested change

Comment on #238 Handle group messages

Extra `event` given to `getValidArticleReply` and most of `getValidArticleReply ()` below

Comment on #238 Handle group messages

We can directly use `categoryId` to save 1 category look-up query. Suggested change Note: we should change `category.id` to `categoryId` below if we want to take this change.

Comment on #238 Handle group messages

nit: give this test case a different name so that its snapshot is easier to find

Comment on #388 Implement tutorial page

I am seeing that some image files are &gt; 50KB. I suggest using <https://imageoptim.com/mac|ImageOptim> or pngquant (uses the same core as TinyPNG, but is easier to use since they run on local machine) and perform lossy compression on all committed image files first.

#241 Send reply list when article URL is sent

This PR will make chatbot return list of replies when a URL to article page is sent to Cofacts chatbot. This should make 3rd party chatbot integration easier. cc/ <https://github.com/CarolHsu|@CarolHsu> *Screenshots* (`SITE_URL` setting on my local machine is `<https://cofacts.hacktabl.org>`, thus it's triggered by that. On production it should be `<https://cofacts.org>`.) <https://user-images.githubusercontent.com/108608/107848774-1e8de280-6e31-11eb-98a6-779134fdbf25.png|image>

Comment on #241 Send reply list when article URL is sent

*Pull Request Test Coverage Report for <https://coveralls.io/builds/37108703|Build 1262>* • *4* of *4* *(100.0%)* changed or added relevant lines in *1* file are covered. • No unchanged relevant lines lost coverage. • Overall coverage increased (+*0.04%*) to *86.459%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*

Review on #241 Send reply list when article URL is sent

Comment on #388 Implement tutorial page

Since we do not need to trigger re-render when memorizing `timeoutId`, we can just use a ref to store `timeoutId` instead.

Comment on #388 Implement tutorial page

Please add <https://nextjs.org/docs/api-reference/next/link#if-the-child-is-a-custom-component-that-wraps-an-a-tag|`passHref`> so that `Button` can be set a `href` prop, turning it to be an tag. This enables users to cmd-click / long-click open new wondow for the button. Suggested change

Comment on #388 Implement tutorial page

Suggest • Remove context (`c`) because we probably want to use the same translation if it's used elsewhere • Capitalize and make it look like a short sentence for better SEO Suggested change

Review on #388 Implement tutorial page

Thanks for the PR. Happy new year! I have added some suggestions regarding react-router `&lt;Link&gt;` and my thoughts on `Slider` props design. Review progress: <https://user-images.githubusercontent.com/108608/107853539-a7b51180-6e51-11eb-81fd-e4c362e7531b.png|image>

Comment on #388 Implement tutorial page

Suggest • Remove context (`c`) because we probably want to use the same translation if it's used elsewhere • Capitalize to make it look like a short sentence for better SEO Suggested change

Comment on #388 Implement tutorial page

As described in <https://reactjs.org/docs/hooks-reference.html#useimperativehandle|ReactJS doc>, imperative code using ref (such as calling `slideTo` outside of `Slider`) should be avoided. We can pick one of the following instead: 1. Make `&lt;Slider&gt;` become an uncontrolled component; Move active indicator inside `&lt;Slider&gt;` component. Anything outside `&lt;Slider&gt;` cannot determine which page to set active. 2. Make `activeIndex` be an optional prop instead. • If `activeIndex` is given, `&lt;Slider&gt;` acts as a controlled component, simply reflecting `activeIndex` and use `onSlideChange` to notify active index change from within the component. • If `activeIndex` is not given, `&lt;Slider&gt;` acts as an uncontrolled component, maintaining its own `activeIndex` as a state. I suggest 2, because it gives better flexibility and we actually only have 1 place that needs to be controlled component (when indicator exists).

Comment on #388 Implement tutorial page

Same as above. It's <https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies|not safe> to omit functions from dep arrays. As for `timeoutId`, it should really be a ref instead of a state. If it were a ref, we don't need to list it in dependency array either, as `ref.current` always points to the latest value, and we will never get a stale ref value coming from some unknown closure. Suggested change

Comment on #388 Implement tutorial page

I suggest we just add `initIndex` and `slideTo` to deps array and remove this hook. Adding these two dep should not break the whole thing, while disabling this may cause problem when we add code to this `useEffect` in the future. Suggested change

Comment on #388 Implement tutorial page

Please consider using existing `&lt;Ribbon&gt;` instead of SVG. You can modify `&lt;Ribbon&gt;` to support blue background.

Review on #245 Add user agreement files

Comment on #238 Handle group messages

有點難描述，我用中文好了 哈哈 這裡要測試 expireQueue 執行時，如果有新的 event 進來，expireQueue 要被暫停，並且執行完 jobQueue 後再繼續，利用監聽 expiredJobQueue 的 active event 加上 `concurrency` 設成 `1`，我們可以確保在 expiredJobQueue 執行期間，插入一個沒過期的 job。 1. 因為這裡把 `GroupHandler` 的 `concurrency` 設成 `1`，所以四個 expiredJob 會在 jobQueue 的 process function <https://github.com/cofacts/rumors-line-bot/pull/238/files/a04a06cccbb584af1d0e3e946e6e8c1b4ff012db#diff-eea0db6bb6c18528afc0978099a6d13f4529cf3f945e332e7af1749204c39d92R38|`processJob`> 一個一個(不保證順序)處理並加進 expiredJobQueue，這裡並不會執行到 `processJob` 裡面的 `processGroupEvent`。 2. 當所有(四個) jobQueue 的 job 被執行完之後(`onDrained`)，expiredJobQueue 會開始(resume)執行。 3. 當第一個 expired job 開始執行時，會同時送出一個 expiredJobQueue 的 job active event。 4. 在收到 expired job active event 的時候加入一個沒過期的 job (到 jobQueue)，並且為了讓它成功，把 `processGroupEvent` 改成 resolve，在 jobQueue on('completed' 改回來。 其實這邊我期望是可以 mockImplementationOnce(expired x 4 + success x 1)，但是 `processGroupEvent` 被執行的順序可能是 • testExpiredId1 -&gt; (expiredJobQueue active event) -&gt; successJobId -&gt; testExpiredId2 -&gt; ... • testExpiredId1 -&gt; (expiredJobQueue active event) testExpiredId2 （幾乎同時）-&gt; successJobId -&gt; ... 因為我們不能保證 active event 收到的時機，不知道 success mock 要放在第幾個順位才能正確讓 successJobId 是 resolve 的，所以就在收到第一個 expiredJobQueue (job) 的 active event 時把它改成 resolve，在它完成時再改回 reject，這樣就會造成一個問題是：把 `processGroupEvent` 改成 resolve 時剛好 testExpiredId2 開始了 expiredJobQueue 還沒 pause，所以 testExpiredId2 的結果就是 complete 的(理論上 expiredJobQueue 的 job 要在 `processGroupEvent` 裡面全被 reject) 這裡只 expect jobQueue 的 success/fail count，因為 expiredJobQueue 的結果可能每次都不一樣

#245 Add user agreement files

Comment on #238 Handle group messages

感謝感謝 我在想如果「這裡要測試 expireQueue 執行時，如果有新的 event 進來，expireQueue 要被暫停，並且執行完 jobQueue 後再繼續」 若把下面這兩塊拆成兩個分開的 test case，會不會比較簡單 1. `expireQueue` 執行時，如果有新的 event 進來，`expireQueue` 要被暫停 --&gt; 先往 `expireQueue` 塞東西，再 `gh.addJob`，然後 expect `expireQueue` 的 status 2. 執行完 `jobQueue` 後再繼續 --&gt; 先準備一個暫停的 expireQueue，然後 `gh.addJob`，然後在該 job 完成之後 expect expireQueue 的 status 是否不是 pause

Review on #241 Send reply list when article URL is sent

LGTM!

#241 Send reply list when article URL is sent

Review on #388 Implement tutorial page

2nd batch of review comments. All previous commits have been reviewed. Regarding the two new pages, I come up with new URLs that is more commonly used in other websites: • `/what-is-cofacts` --&gt; `/about` • `/how-to-use` --&gt; `/tutorial`

Comment on #388 Implement tutorial page

Since the height of the content wrapper is fixed, it's possible that the content needs scrolling. In this case, I think we should not hide the scrollbars. I suggest we style the scrollbar so that the track is transparent, but make the scrollbar thumb still visible.

Comment on #388 Implement tutorial page

I think we can keep it simple, just use `fileMock` for the 4 file formats

Comment on #388 Implement tutorial page

Please use <https://material-ui.com/components/tabs/#nav-tabs|Material-UI nav tabs> + &lt;<https://nextjs.org/docs/api-reference/next/link%7Cnext.js> `&lt;Link&gt;`&gt; (may need to turn on `passHref` to work properly) for each tab element so that • web crawlers can recognize that this is a link and follow through • users can cmd-click / long-press to open tab in new browser tab • clicking on tab can have ripple effect

Comment on #388 Implement tutorial page

Suggest using `&lt;a&gt;` here so that the user will be able to cmd-click / long-press to open in new browser tabs, and it will also work for web crawlers to successfully follow the links

Comment on #388 Implement tutorial page

The color have a name "Blue 2" on figma: <https://user-images.githubusercontent.com/108608/107931593-722a3880-6fb7-11eb-8689-a8d275bdb26a.png|image> It's available via `theme.palette.common`. Suggested change

Comment on #388 Implement tutorial page

Nit: we can use `nav` HTML tag so that web crawlers _may_ recognized that it contains navigational links :stuck_out_tongue:

#242 Handle the case when article id is not found

Since we support third-party chatbots sending URLs to trigger `choosingArticle`, we should better handle exceptions, especially for the case when article id is not correct (user typed in extra message before sending it to Cofacts) <https://user-images.githubusercontent.com/108608/108032616-b841d380-706d-11eb-82a5-56b4c0ca686d.png|image>

Comment on #242 Handle the case when article id is not found

*Pull Request Test Coverage Report for <https://coveralls.io/builds/37159288|Build 1267>* • *2* of *2* *(100.0%)* changed or added relevant lines in *1* file are covered. • No unchanged relevant lines lost coverage. • Overall coverage increased (+*0.04%*) to *86.498%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*

#389 Update ttag babel plugin

to fix known issue in <https://github.com/cofacts/rumors-site/pull/387|#387>

Review on #389 Update ttag babel plugin

#389 Update ttag babel plugin

#246 Revamp index page to display legal &amp; usage

• Remove `checkConsent` and its corresponding env var `LICENSE_URL` • Will no longer apply • index contains link to `LEGAL.md` • index contains instructions on how to query API <https://user-images.githubusercontent.com/108608/108099286-5e69f980-70bf-11eb-9750-79b1d26d08ea.png|image>

Comment on #246 Revamp index page to display legal &amp; usage

<https://coveralls.io/builds/37176645|Coverage Status> Coverage increased (+0.5%) to 86.578% when pulling *<https://github.com/cofacts/rumors-api/commit/81f8a19bffe78bfd9a2d57d508745cc56230fcd5|81f8a19> on display-legal* into *<https://github.com/cofacts/rumors-api/commit/0d6990c171402f8da7857aa79097da68ad45c458|0d6990c> on master*.

#242 Handle the case when article id is not found

Since we support third-party chatbots sending URLs to trigger `choosingArticle`, we should better handle exceptions, especially for the case when article id is not correct (user typed in extra message before sending it to Cofacts) <https://user-images.githubusercontent.com/108608/108032616-b841d380-706d-11eb-82a5-56b4c0ca686d.png|image>

Review on #242 Handle the case when article id is not found

#242 Handle the case when article id is not found

Review on #246 Revamp index page to display legal &amp; usage

#390 Content too long and cause horizontal scroll on mobile

Discovered at <https://g0v.hackmd.io/@mrorz/cofacts-meeting-notes/%2FJUVhhxSmSx62O5SUVuJ1rw|20210120> release check. Ex: <https://dev.cofacts.org/article/37rwpsmfuqeiz|https://dev.cofacts.org/article/37rwpsmfuqeiz> <https://user-images.githubusercontent.com/108608/108160020-344a2300-7123-11eb-9d5f-3f9fcfa22e89.png|image>

#246 Revamp index page to display legal &amp; usage

Comment on #388 Implement tutorial page

Thanks for the update! I have resolved comments that have been implemented. What's left are: • `tutorial.js` image tab (`tabContainer`) should use `&lt;a&gt;`; consider using semantic tag `nav` for `tabContainer` • use `fileMock` and remove `styleMock`

Comment on #238 Handle group messages

*Pull Request Test Coverage Report for <https://coveralls.io/builds/37194303|Build 1270>* • *116* of *132* *(87.88%)* changed or added relevant lines in *8* files are covered. • No unchanged relevant lines lost coverage. • Overall coverage increased (+*0.3%*) to *86.709%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*

Comment on #238 Handle group messages

Sorry, the comment might come from copy and paste and I forgot to remove it.

Review on #238 Handle group messages

Seems that most issues are resolved. Things left are 1. documentation regarding the custom dimensions; and if we should use custom metric instead of custom dimension for user count in chat groups 2. excessive comment 3. simplify test case I think we can discuss on 1 because setting custom metric / dimension cannot be undone in the future. Documentations, comments &amp; tests can wait.

#243 Update Liff redirect method to concatenate mode

Fixes <https://github.com/cofacts/rumors-line-bot/issues/239|#239> • From <https://developers.line.biz/console/channel/|developer console> Switch `Method for converting additional information in the LIFF URL` to `Concatenate` • Update richmenu usersetting url to `LIFF_URL?p=setting`

Comment on #243 Update Liff redirect method to concatenate mode

*Pull Request Test Coverage Report for <https://coveralls.io/builds/37199483|Build 1274>* • *1* of *1* *(100.0%)* changed or added relevant line in *1* file are covered. • No unchanged relevant lines lost coverage. • Overall coverage remained the same at *86.498%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*

Review on #243 Update Liff redirect method to concatenate mode

Let's try it out tonight!

Comment on #238 Handle group messages

We will merge first and just setup 1st custom dimension. Leave 2nd dimension / metric for the future.

#13 支援 LINE chatbot 群組聊天

#238 Handle group messages

Review on #388 Implement tutorial page

LGTM! We will track items left in a separate ticket

#388 Implement tutorial page

#391 Implement final report page

UI: <https://www.figma.com/file/mfsTnlvaMC1MC4aG8tf5YW|https://www.figma.com/file/mfsTnlvaMC1MC4aG8tf5YW>

Review on #15 Refactor nginx config and add SEO headers

Review on #15 Refactor nginx config and add SEO headers

m.facebook.com

LINE Developers Taiwan

【LINE 更新報你知 :loudspeaker:】 穿新衣:necktie:～戴新帽:billed_cap:～ LIFF 升級新版本，也要帶給大家新的體驗啦！ 快快跟上更新的腳步，才能避免出問題:warning: 不要讓年初就輸在起跑點上:runner: 重點整理看這裡 :arrow_down::arrow_down::arrow_down: :four_leaf_clover:項目：LIFF SDK :four_leaf_clover:版本：2.8.0 :four_leaf_clover:時間：2021/02/15 :four_leaf_clover:重點整理： - 取消 LIFF 二次跳轉前的 liff.init() reslove - query...

#347 SEO for Mandarin content

#15 Refactor nginx config and add SEO headers

Review on #391 Implement final report page

LGTM! We can merge after compressing the images.

Comment on #391 Implement final report page

Suggested change

Review on #391 Implement final report page

Update youtube link

#391 Implement final report page

Comment on #1 project init

to see <https://github.com/cofacts/rumors-ai/issues/10|cofacts/rumors-ai#10>

#2 Environment Setting

☐ test (staging) ☐ production

#13 write finished tasks back to elasticsearch

Comment on #9 README refinement

<https://github.com/cofacts/rumors-ai#run|https://github.com/cofacts/rumors-ai#run> <https://github.com/cofacts/rumors-ai#environment-variables|https://github.com/cofacts/rumors-ai#environment-variables>

#6 Prepare task (testdata) for new model

Comment on #6 Prepare task (testdata) for new model

<https://github.com/cofacts/rumors-ai/commit/753f6105832cf8d34fbe8a69a7a0394e6588f30d|753f610>

#392 Sponsor

#393 [LandingPage] change links to tutorial instead

#244 Fix group message issues

Fix remain issues from PR <https://github.com/cofacts/rumors-line-bot/pull/238|#238> 1. documentation regarding the custom dimensions; and if we should use custom metric instead of custom dimension for user count in chat groups 2. remove excessive comment 3. simplify test case Others 1. Add leave command for room user to kick out chatbot 2. Fix rollbar item 335 [LINE Client] 400: Only group id is acceptable (room/group use different api endpoint to get members count)

Comment on #244 Fix group message issues

*Pull Request Test Coverage Report for <https://coveralls.io/builds/37323490|Build 1284>* • *13* of *13* *(100.0%)* changed or added relevant lines in *2* files are covered. • No unchanged relevant lines lost coverage. • Overall coverage increased (+*0.07%*) to *86.776%* * * * * * * *:yellow_heart: - <https://coveralls.io|Coveralls>*

Comment on #244 Fix group message issues

Because chatbot may join/leave a group multiple times, we should find the last `Join` action of the `Client Id` to get a more accurate `Group Members Count`. Don't know if there's a better way to handle this condition.

#392 Sponsor

#393 [LandingPage] change links to tutorial instead

#394 Move report page to /impact

As discussed with <https://github.com/bil4444|@bil4444> we want to move report page URL from `/report` to `/impact` • This URL is year neutral; can re-use every year • Means "social impact", which is more specific than "report" <https://user-images.githubusercontent.com/108608/108721818-66adb300-755d-11eb-800e-760599a852ae.png|image>

Review on #394 Move report page to /impact

Review on #393 [LandingPage] change links to tutorial instead

Review on #393 [LandingPage] change links to tutorial instead

Review on #392 Sponsor section in report page

Review on #392 Sponsor section in report page

#393 [LandingPage] change links to tutorial instead

#392 Sponsor section in report page

#394 Move report page to /impact

#14 reset requirements.txt

There is some redundancy.

#1 project init - model-config

Comment on #1 project init - model-config

<https://github.com/cofacts/rumors-ai-bow/commit/62ded83511d5fa2b2ab0d049ca0410aada42a38c|62ded83>

Comment on #2 Environment Setting

<https://github.com/cofacts/rumors-ai-bow/commit/2d2738b03bddc285807a37d3101cb92858fd3a71|2d2738b>

#357 support openpeeps avatar

mrorz 2021-02-23 18:30:40

驚
what happened

zoe 2021-02-23 20:40:43

我想說跟edit 一起進去好了，我拆不開QQ

• 1

#395 Edit avatar

continuing from <https://github.com/cofacts/rumors-site/pull/357|#357>

#247 minor avatar related changes

• change default image size to 100 • add image size to facebook url • omit avatarData field on non-openpeeps updates, instead of wiping it

Comment on #247 minor avatar related changes

<https://coveralls.io/builds/37380694|Coverage Status> Coverage increased (+0.007%) to 86.479% when pulling *<https://github.com/cofacts/rumors-api/commit/2902b76d45a5978e93ff3f7548f1b06f68536370|2902b76> on avatars* into *<https://github.com/cofacts/rumors-api/commit/d679bc118185a4df2dcc5dbda464b548a82629c8|d679bc1> on master*.

#396 Facebook timeline-like time display

discussion: <https://g0v.hackmd.io/CqKV8gfNSfyuvdQOtq7TlQ#%E6%99%82%E9%96%93%E9%A1%AF%E7%A4%BA|https://g0v.hackmd.io/CqKV8gfNSfyuvdQOtq7TlQ#%E6%99%82%E9%96%93%E9%A1%AF%E7%A4%BA> *as-is* All time display is relative to current time. It was set like this because in server-side rendering, the time zone is GMT, this relative time was chosen by default. *to-be* Use facebook-like time display as discussed in <https://g0v.hackmd.io/CqKV8gfNSfyuvdQOtq7TlQ#%E6%99%82%E9%96%93%E9%A1%AF%E7%A4%BA|https://g0v.hackmd.io/CqKV8gfNSfyuvdQOtq7TlQ#%E6%99%82%E9%96%93%E9%A1%AF%E7%A4%BA> . For time zone issue, we can • keep ISO Timestamp in `&lt;time&gt;` HTML property so that search engine can read the time correctly • apply absolute time display on component mount, which should only trigger on browsers

#397 Reorder categories so that marked and categories from similar articles can appear earlier

*As-is* The category dialog sorts the category using arbitrary order (same order the API has returned) <https://user-images.githubusercontent.com/108608/108950382-1dff1280-76a1-11eb-9be4-9e491846c290.png|image> *To-be* Per discussed in <https://g0v.hackmd.io/CqKV8gfNSfyuvdQOtq7TlQ#%E6%8E%A8%E8%96%A6%E6%A8%99%E7%B1%A4|20210217>, we should • Move categories that has been added to the front • Then list out categories that is also marked in similar articles • Then the rest of the categories.

Comment on #244 Fix group message issues

Got it, thanks for the note!

Comment on #244 Fix group message issues

Does this trigger leave webhook? If not, we may need to manually send `ga` here.

Review on #244 Fix group message issues

Thanks for the fix! Let's get it to dev &amp; staging ASAP.

#244 Fix group message issues

Comment on #247 minor avatar related changes

nit: since we do not clear `avatarData` when openpeeps is not chosen, I think we can update the test name accordongly.

Review on #247 minor avatar related changes

Thanks for the change!

#15 exceptional handling for data syncing

data syncing fails sometimes and stops the whole process

#243 Update Liff redirect method to concatenate mode

Comment on #244 Fix group message issues

Yes, it does.

• 🦒1

• 1

#239 Update LIFF redirect option

#243 Update Liff redirect method to concatenate mode

#247 minor avatar related changes

#398 Implement banner animation and translate impact page

Implement banner animation and translate impact page

Comment on #395 Edit avatar

We probably want to name this method something like `sanitizeAvatarData` as it returns an object rather than true / false or throws on invalid object.

Review on #395 Edit avatar

<https://user-images.githubusercontent.com/108608/109265584-bfbd6580-7841-11eb-9a7b-e823b5875315.png|image> Some early comments so far

Comment on #395 Edit avatar

This will create 3 levels of div around the `svg`. The inner most div is created by `circleStyle` in `Peep` (relevant code: <https://github.com/CeamKrier/react-peeps/blob/master/src/peeps/index.tsx#L160|https://github.com/CeamKrier/react-peeps/blob/master/src/peeps/index.tsx#L160> ) I think we can 1. Just wrap 1 div around `&lt;Peep&gt;` 2. put styles in `peepsStyles.circleStyle` to `classes.showcaseWrapper`. 3. remove `circleStyle` prop from `&lt;Peep&gt;` to prevent `react-peeps` from generating extra div that

Comment on #395 Edit avatar

Since `JSON.parse` can be expensive to do in render functions, I suggest isolate it in `useMemo`: Suggested change

Comment on #395 Edit avatar

Suggest using <https://reactjs.org/docs/hooks-reference.html#lazy-initial-state|lazy initialization> since `getInitialAvatarData` is a bit expensive to do on every render Suggested change

Review on #395 Edit avatar

Thanks for the avatar edit function, it's a real fun to see these vibrant avatars! I have some suggestions regarding data loading &amp; DOM structuring, please take a look

Comment on #395 Edit avatar

Given the fact that • All data in this fields are only required when EditAvatarDialog is opened; they are not required to render a profile page • SSR for these fields will never work due to lack of session cookies I think it makes for sense if we perform a separate `useQuery` right inside render function of `EditProfileDialog` (i.e. after `EditProfileDialog` is opened ) instead of putting its fragment in `UserPageHeader`'s `useQuery`. Plus, we could also drop `loadSelfOnlyFieldsForUser` lazy query &amp; `useEffect` in `ProfilePage` if we load this fragment in `EditProfileDialog`. (Seems that I should the similar to `EditProfileDialogUserData` as well; maybe I will refactor that in the future)

Comment on #395 Edit avatar

It seems that these fields are only used in `EditAvatarDialog`. In this case I suggest adding these fields to `EditAvatarDialogUserData` fragment in `EditAvatarDialog` instead.

Comment on #395 Edit avatar

I think we can add <https://www.apollographql.com/docs/react/data/mutations/#options|`refetchQueries: ['LoadProfilePage']`> to `useMutation(UPDATE_USER)` call in `EditAvatarDialog` instead. In this way, profile page data can be reloaded after user submits avatar.

Comment on #395 Edit avatar

Seems that removing this div can make mobile look even better; buttons can wrap and all buttons become clickable <https://user-images.githubusercontent.com/108608/109304728-a33c2000-7877-11eb-9ea6-e4eaa809b6bd.png|image> And on desktop it looks decent as well.

Comment on #395 Edit avatar

When `ComponentInput` is rendered, `wrapperEl.current` is null; after `AvatarSelector` is rendered, `wrapperEl` is populated, but that does not trigger re-render. `anchorEl` <https://material-ui.com/api/menu/|supports a function>. I think when a function is passed, material-ui will invoke it when menu is opened, thus this can fetch the latest ref value in scope. Suggested change Ref object returned by `useRef` is always an object that has `currrent` property (even just after initialization, it's `{current: null}`) so no need for `?.`. The same applies to `&lt;ColorPicker&gt;` below.

Review on #398 Implement banner animation and translate impact page

LGTM! Thanks for improving the impact page!

Comment on #398 Implement banner animation and translate impact page

nit: I think we can use media query for `bgBottom`; in this way we don't need to use Javascript (`isMobile`).

Comment on #398 Implement banner animation and translate impact page

Let's merge first to resolve conflict on i18n translations

#398 Implement banner animation and translate impact page