cofacts

HackMD

在 LINE Chatbot 中串接 Google Analytics 的經驗分享 - HackMD

在 LINE Chatbot 中串接 Google Analytics 的經驗分享

1 1 1

#189 Date util

small refactor, moving date range helper to util to be reused in analytics dataloader

#190 Analytics data loader

depending on <https://github.com/cofacts/rumors-api/pull/188|#188> and <https://github.com/cofacts/rumors-api/pull/189|#189>

Comment on #189 Date util

<https://coveralls.io/builds/32483673|Coverage Status> Coverage decreased (-0.02%) to 87.574% when pulling *<https://github.com/cofacts/rumors-api/commit/839bcd08f441b5d18d1b6e0015c6545c6a5e3b0f|839bcd0> on dateUtil* into *<https://github.com/cofacts/rumors-api/commit/7dc45064a84a9ae4e129d45dc77d1ff44270ae23|7dc4506> on fetchGA*.

Comment on #190 Analytics data loader

<https://coveralls.io/builds/32483677|Coverage Status> Coverage increased (+0.2%) to 87.78% when pulling *<https://github.com/cofacts/rumors-api/commit/095d1eaa1fad7697f9425e609889d5bd740c84fa|095d1ea> on analyticsDataLoader* into *<https://github.com/cofacts/rumors-api/commit/839bcd08f441b5d18d1b6e0015c6545c6a5e3b0f|839bcd0> on dateUtil*.

#191 Article stats

Comment on #191 Article stats

<https://coveralls.io/builds/32483773|Coverage Status> Coverage increased (+0.02%) to 87.798% when pulling *<https://github.com/cofacts/rumors-api/commit/969bc5b810aadad8c8722af076eee796e8093513|969bc5b> on articleStats* into *<https://github.com/cofacts/rumors-api/commit/095d1eaa1fad7697f9425e609889d5bd740c84fa|095d1ea> on analyticsDataLoader*.

Review on #188 script to fetch google analytics stats

Thanks for the commit! I have finished commenting on the script implementation. Going to inspect the test cases tonight.

Comment on #188 script to fetch google analytics stats

Since this try-catch statement covers the entire function, it is equivalent to doing try-catch in the call site of `upsertDocStats`. However, the latter allows the user of `upsertDocStats` to determine if they want to catch it. I would suggest removing try-catch here.

Comment on #188 script to fetch google analytics stats

I find it difficult to differentiate the use case between L291-294 and L307-315. A comment elaborating this case (edge-case handling across batches) would be helpful.

Comment on #188 script to fetch google analytics stats

dotenv variables should be string already, thus `''` can be omitted. For example, `process.env.PORT` is `5000` even without `''`.

Comment on #188 script to fetch google analytics stats

Seems that errors are all unexpected items that should be reported to Rollbar. It would be nice if we send them using `Rollbar.error(err)`.

Comment on #188 script to fetch google analytics stats

Some explanation on how to execute the script here (or in `README.md` instead) would be great. Newly introduced env vars in `.env.sample` needs some documentation in here (or in `README.md` instead).

Comment on #188 script to fetch google analytics stats

I suppose `row.metrics[0].values` are numbers in strings? If so, `parseInt(visits, 10)` or even `+visits` can convert strings to number. Use of eval is <https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/eval#Never_use_eval!|highly discouraged>.

Comment on #188 script to fetch google analytics stats

Since it's <https://developers.google.com/analytics/devguides/reporting/core/v4/resource-based-quota|analytics 360 only function>, we will probably never uncomment it. Let's remove this line~

Comment on #188 script to fetch google analytics stats

Seems that errors are all unexpected items that should be reported to Rollbar. It would be nice if we send them using `Rollbar.error(err)`.

Comment on #188 script to fetch google analytics stats

I am wondering if <https://docs.oracle.com/en/java/javase/11/docs/api/java.base/java/util/Map.html#putAll(java.util.Map)|`putAll()`> is useful here. I also like the current implementation, which explicitly lists all changed fields, this makes modifications crystal clear :gem: .

Review on #189 Date util

Thanks! I've added a minor suggestion regarding the use of `isValid`.

Comment on #189 Date util

Personally I prefer throwing errors with message text for these utility functions. Pros: • removes the need to do `else` block and simplifies code structure a lot. • have the same expressiveness of error message + `isValid` • when it's invalid, only `error` field is relevant. `isValid` is derived from `error`, and `startDate` / `endDate` are simply not applicable. • in this case, an exception with message can fully express the same amount of information. Cons: • `validateOOO` functions do imply returning error instead of throwing them. • may need to rename to something like `assertOOO`

Comment on #188 script to fetch google analytics stats

Another idea that come across my mind is, processing items across batches should be pretty similar to merging different `pagePathLevel2` -- they all memorize the last row and perform update when necessary. This means that in theory we can just implement merging of different `pagePathLevel2`. When batch changes, we should put `prevLastRow` into the "last-row" memory and perform identical logic that handles `pagePathLevel2`. Another way of thinking is that we blur the boundary of "batch" and only work on one row at a time, providing last "row" (no matter it's from same batch or previous batch) during the process.

116155217_350793832748131_7278387044960444471_n.jpg

116207176_1231006320590790_4735981624252461450_n.jpg

116212778_216506436350194_8151330362891010884_n.jpg

116235900_378582476456256_530460603154245394_n.jpg

116800306_291163598653757_6610934379152638743_n.jpg

117068519_327122458671111_816188199161369806_n.jpg

mrorz 2020-08-03 12:12:13

很可愛～～～

那個光芒是 for 「首次」的嗎
感覺首次好像比「999次」還要吸引人 XD

stbb1025 2020-08-03 12:13:35

對～首次的XD 999k次是因為之後會隨數字level不同改顏色，所以目前比較陽春一點XD，不過我可以跟熊討論一下看要怎樣也很吸引人

mrorz 2020-08-03 13:10:11

或是把「首次」的調得不那麼吸引人 (?)
把萬丈光芒換到更高級的 badge 之類

stbb1025 2020-08-03 13:11:03

嗯嗯那要訂個level哈哈
不過我覺得首次可以多點鼓勵也不是壞事XD

bil 2020-08-03 13:18:41

好可愛～～～

stbb1025 2020-08-06 15:27:38

@mrorz 關於很多次數的尊榮感，有辦法給我一個每個項目「大於多少會很屌」的數字嗎？

mrorz 2020-08-06 17:01:47

要請 @bil 針對這個文件裡提到的成就來決定一下很酷的 `n` 值
https://g0v.hackmd.io/bbreV0ZqRDarHl4Zt5UpEw#%E6%88%90%E5%B0%B1

mrorz 2020-08-06 17:04:59

連續上站天數 > 7 我覺得就很厲害了

但如果連續上站天數 > 100
感覺像是以前「連續 100 天晚餐開飯挑戰」的日本節目
或許該要有獎金 (欸

bil 2020-08-09 19:57:27

那就n的費氏數列或是以每25為一個界線

3 🐳 2 🦒 2

Comment on #304 Fix 262 rss function

I was thinking if we can directly convert `listQueryVars` to JSON and put them in feed URL. That should simplify `api/articles/[feed]` (because it don't need `getQueryVars`, just `JSON.parse`) and allows greater flexibility for the feed. What do you think <https://github.com/nonumpa|@nonumpa> ?

Comment on #304 Fix 262 rss function

It's brilliant that we can remove default option from `getQueryVars()`. It makes the code easier to read :+1: I am thinking if we can further simplify this to: ``` const listQueryVars = getQueryVars({ // Defaults that can be overridden by query filters: defaultFilters.join(','), orderBy: defaultOrder, // URL query ...query, // Others to join query timeRangeKey, userId: user?.id, }); ```

Review on #304 Fix 262 rss function

Thanks for the fix! I have attached 2 comments at first glance. Will look at other parts tonight.

Comment on #190 Analytics data loader

Elasticsearch <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/query-dsl-range-query.html#ranges-on-dates|range query> actually supports <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/common-options.html#date-math|date math> for date fields. Currently several inputs like `ListArticleFilter.createdAt`, `ListArticleFilter.repliedAt` leverages `TimeRangeInput` to achieve date range inputs that supports date math, which is essentially exposing the whole range query filter and letting Elasticsearch check the format directly. I am thinking if we can do the same for `analyticsLoader`. I am OK with open ends (For example, with `gte` given but no `lte`) query since Elasticsearch always bound its result to a certain <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/search-request-from-size.html|size>.

Review on #190 Analytics data loader

Thanks for implementing the analytics data loader! I have raised some questions on the `analyticsLoaderFactory` and hope we can discuss before merge.

Comment on #190 Analytics data loader

Since the `ID` used for this data loader is not primitive, we may need to define `cacheKeyFn` so that dataloader can tell if any "id"s given are identical. This is useful for caching and batching. Example: <https://github.com/cofacts/rumors-api/blob/master/src/graphql/dataLoaders/docLoaderFactory.js#L21|https://github.com/cofacts/rumors-api/blob/master/src/graphql/dataLoaders/docLoaderFactory.js#L21> Documentation: <https://github.com/graphql/dataloader#new-dataloaderbatchloadfn--options|https://github.com/graphql/dataloader#new-dataloaderbatchloadfn--options>

Comment on #190 Analytics data loader

Elasticsearch body search by <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/search-request-from-size.html|returns 10 results by default>. This may cause this dataloader never return more than 10 results. We may want to relax that using <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/search-request-from-size.html|`size` param>. Personally I think we can just set a reasonable constant here (Maybe 30 / 60 / 90?). I am ok with crazy values as well -- `articleReplyFeedbackLoader` uses 10,000 lol

Review on #189 Date util

LGTM. Thanks for the change!

#305 RSS UI

figma <https://www.figma.com/file/YikWPPauMnukDH0U2Nq4YS/Components?node-id=1%3A310|https://www.figma.com/file/YikWPPauMnukDH0U2Nq4YS/Components?node-id=1%3A310> <https://user-images.githubusercontent.com/6376572/89249900-ffbec000-d645-11ea-8880-78fbb9516f8e.gif|desktop-zh> <https://user-images.githubusercontent.com/6376572/89250032-5b894900-d646-11ea-8923-a9c532abda4b.gif|mobile-en> *Subscribe through IFTTT* <https://user-images.githubusercontent.com/6376572/89250041-62b05700-d646-11ea-8b9e-f4023ffc8d20.png|image> Not Ready: 1. Icon (currently download and search from google image) 2. Copy rss feed button is better to popup a tip 'copied to clipboard'.

Comment on #188 script to fetch google analytics stats

• nit: default

Comment on #188 script to fetch google analytics stats

It would be neat if we have comment noting that this block may (implicitly) modify entries that is pushed into `bulkUpdates` in previous loop.

Review on #188 script to fetch google analytics stats

Thanks for the fix. Please allow me to spend more time inspecting test cases. I need more time to verify if `processReport`is calling `upsertDocStats` with correct `bulk` for the given mocked `rows`.

Review on #188 script to fetch google analytics stats

Finished looking at the test cases of `processReport` and `upsertDocStats`. They look great :)

Comment on #188 script to fetch google analytics stats

If we are going to repeat the `source` of the script many many times in a batch, I think we may consider using stored scripts. <https://www.elastic.co/guide/en/elasticsearch/reference/6.8/modules-scripting-using.html#modules-scripting-stored-scripts|https://www.elastic.co/guide/en/elasticsearch/reference/6.8/modules-scripting-using.html#modules-scripting-stored-scripts> We can update the stored script everytime the cron job starts to make sure the script stored in database is the latest one.

#192 Hide obsolete categories

From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#Category-%E6%A8%99%E8%A8%98-UI-%E6%AA%A2%E8%A8%8E|20200729> We should hide obsolete categories from users, but leave their articleCategories in tact (or back it up somewhere)

#306 Improve category dialog

From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#Category-%E6%A8%99%E8%A8%98-UI-%E6%AA%A2%E8%A8%8E|20200729>: The add-category dialog is now too long and requires a lot of scrolling. Solution TBA.

#307 List 25 items per page in list page UIs

From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#%E3%80%8C%E7%AD%89%E4%BD%A0%E4%BE%86%E7%AD%94%E3%80%8D%E7%9A%84%E5%95%8F%E9%A1%8C|20200729> Currently article list page only show 10 item at a time. We should increase the number to 25 to reduce the need to click "Load more".

#308 Show URL title in article list

From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#%E3%80%8C%E7%AD%89%E4%BD%A0%E4%BE%86%E7%AD%94%E3%80%8D%E7%9A%84%E5%95%8F%E9%A1%8C|20200729> We can display URL's title text in article list. Figma: • Desktop <https://www.figma.com/file/DvmAQjMJCncuPORWKnljM1/Cofacts-website-MrOrz-s-copy?node-id=2675%3A221|https://www.figma.com/file/DvmAQjMJCncuPORWKnljM1/Cofacts-website-MrOrz-s-copy?node-id=2675%3A221> • Mobile <https://www.figma.com/file/DvmAQjMJCncuPORWKnljM1/Cofacts-website-MrOrz-s-copy?node-id=2673%3A220|https://www.figma.com/file/DvmAQjMJCncuPORWKnljM1/Cofacts-website-MrOrz-s-copy?node-id=2673%3A220>

#193 Reload preview for logged in users

From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#%E5%A4%A7%E6%9D%BE%E6%AA%A2%E8%A8%8E|20200729> Sometimes our URL scrapper will be temporarily blocked, leaving useless URL previews like <https://cofacts.org/article/39n5zi0t4vlci|this>. During <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#Youtube-%E9%98%BB%E6%93%8B-metadata-scraping|20200729 discussion> we can provide an API for logged in editors to reload broken review. Spec TBA

#309 Show "OK" button in mobile article filter dialog

From <https://g0v.hackmd.io/r3NK7xssSwCNPFxthlw7tA#%E6%9C%AA%E7%AB%9F%E9%A0%85%E7%9B%AE1|20200729>: &gt; 手機上 filter 按 X 會以為是 cancel，實驗之後才發現是按下去就會 apply Figma: "Apply filter" <https://www.figma.com/file/zpD45j8nqDB2XfA6m2QskO/Cofacts-website?node-id=2322%3A0|https://www.figma.com/file/zpD45j8nqDB2XfA6m2QskO/Cofacts-website?node-id=2322%3A0> We can retain the behavior that pressing filter buttons starts filter process immediately (because it's the current behavior and is easier to implement), but add an "OK" button at the bottom of filter dialog so that user can click to close filter dialog.

#310 Reply request submission button jumps around when clicking

*As-is* <https://user-images.githubusercontent.com/108608/89379879-2f92c400-d729-11ea-90f9-bcc7bcea0bbc.gif|reason-submission> 1. After inputting reason, the reason submission button jumps up and cancels the first click 2. During submission, there is no visual feedback indicating that the reply request is being sent *To-be* 1. Don't change reason input height when reason input is blurred 2. Add a loading state (or simply disable the submission button) to reason submission button when it's being submitted to server.

g0v.hackmd.io

20200805 會議記錄 - HackMD

Comment on #304 Fix 262 rss function

I also wanted to remove the dependency of `ArticlePageLayout.getQueryVars` from `api/articles/[feed]`, but it seems that it's not easy to directly convert `listQueryVars` to querystring. <https://user-images.githubusercontent.com/6376572/89401059-5102a880-d747-11ea-8185-e96c7c51b9c3.png|螢幕快照 2020-08-05 下午6 01 23> I'm afraid that I should write a `listQueryVarsToQuerystring` function and another `getQueryVars` for `api/articles/[feed]`.

Comment on #304 Fix 262 rss function

I was thinking a direct `JSON.stringify` should do the job :P Is its result too long for an URL?

mrorz 2020-08-05 18:32:00

其實關於 JSON 塞進 URL
也有人的做法是用 zip 壓縮一發，然後轉 base64 XDD
https://github.com/masotime/json-url

mrorz 2020-08-05 18:34:54

要不然就是複雜的 URL encode QQ
https://github.com/ljharb/qs

討論的文章（但沒給解法）：https://dropbox.tech/developers/json-in-urls

nonumpa 2020-08-05 18:53:10

嗯嗯，我也看到了

mrorz 2020-08-05 20:38:27

原 article list URL: 162 byte (但 `REPLY_BY_ME` 還另外需要 user id)
```filters=REPLIED_MANY_TIMES%2CNO_USEFUL_REPLY_YET%2CREPLIED_BY_ME&types=OPINIONATED&categoryIds=lT3h7XEBrIRcahlYugqq%2Ckj287XEBrIRcahlYvQoS%2CmD2n7nEBrIRcahlYLAr7```
query json：358 byte
```{"filter":{"replyRequestCount":{"GTE":1},"categoryIds":["lT3h7XEBrIRcahlYugqq","kj287XEBrIRcahlYvQoS","mD2n7nEBrIRcahlYLAr7","mj2n7nEBrIRcahlYdArf"],"replyCount":{"GTE":3},"hasArticleReplyWithMorePositiveFeedback":false,"articleRepliesFrom":{"userId":"AVqVwjqQyrDaTqlmmp_a","exists":true},"replyTypes":["OPINIONATED"]},"orderBy":[{"lastRequestedAt":"DESC"}]}```
json + urlencode: 520 byte
```%7B%22filter%22%3A%7B%22replyRequestCount%22%3A%7B%22GTE%22%3A1%7D%2C%22categoryIds%22%3A%5B%22lT3h7XEBrIRcahlYugqq%22%2C%22kj287XEBrIRcahlYvQoS%22%2C%22mD2n7nEBrIRcahlYLAr7%22%2C%22mj2n7nEBrIRcahlYdArf%22%5D%2C%22replyCount%22%3A%7B%22GTE%22%3A3%7D%2C%22hasArticleReplyWithMorePositiveFeedback%22%3Afalse%2C%22articleRepliesFrom%22%3A%7B%22userId%22%3A%22AVqVwjqQyrDaTqlmmp_a%22%2C%22exists%22%3Atrue%7D%2C%22replyTypes%22%3A%5B%22OPINIONATED%22%5D%7D%2C%22orderBy%22%3A%5B%7B%22lastRequestedAt%22%3A%22DESC%22%7D%5D%7D```
json-url (lzma + base64): 332 bytes
```XQAAAAIsAQAAAAAAAABBKYjGk5HEXfadWXKw7jepkVvpHoaw6thdjhrr-b1XehFIp1i0lfNrdYZgFkEbF3OF5gn8efegcgyfPNxJUo1ugsbXwWOwRZ5d67whl8PiANl3KmDjNbsg1xWrJY5S3_cdF2twLn2_u5RKgHrQE-DPp_568wT_wym2FAQlLV6pr-bhqJEwOsubFr4Ih4vp37oJEVyT1kjMcRSLBlEycFbnSkTK9sVPQyWfwMnc5634qeydZhieKsknWJI7U7oWtEfsvqnk5boOkkzM0EVC_PneGyjKvck3KpXqE25WwyyIF411o32v_v_mSwQA```
ljharb/qs: 535 byte
```filter%5BreplyRequestCount%5D%5BGTE%5D=1&filter%5BcategoryIds%5D%5B0%5D=lT3h7XEBrIRcahlYugqq&filter%5BcategoryIds%5D%5B1%5D=kj287XEBrIRcahlYvQoS&filter%5BcategoryIds%5D%5B2%5D=mD2n7nEBrIRcahlYLAr7&filter%5BcategoryIds%5D%5B3%5D=mj2n7nEBrIRcahlYdArf&filter%5BreplyCount%5D%5BGTE%5D=3&filter%5BhasArticleReplyWithMorePositiveFeedback%5D=false&filter%5BarticleRepliesFrom%5D%5BuserId%5D=AVqVwjqQyrDaTqlmmp_a&filter%5BarticleRepliesFrom%5D%5Bexists%5D=true&filter%5BreplyTypes%5D%5B0%5D=OPINIONATED&orderBy%5B0%5D%5BlastRequestedAt%5D=DESC```

Comment on #304 Fix 262 rss function

Sorry, I mean querystring, not JSON. Maybe I can encode filter and orderBy like <https://stackoverflow.com/questions/41641426/passing-objects-as-url-parameters-c-sharp|this>.

#164 replyRequests should return more than 10 entries

#187 Load more reply request at once

Comment on #187 Load more reply request at once

Merged because opened for 10+ days without review.

Comment on #305 RSS UI

Is this file used anywhere?

Comment on #305 RSS UI

Maybe <https://material-ui.com/api/list-item-text/#props|`secondaryTypographyProps`> could help? e.g. ``` &lt;ListItemText primary={t`Email`} secondary={t`Via Feedrabbit`} secondaryTypographyProps={{variant: 'subtitle2', color: 'textSecondary'}} /&gt; ``` Actually I think it's OK if we use Material UI's default style for `&lt;ListItemText&gt;`. The figma link provided by <https://github.com/LucienLee|@LucienLee> is at wireframe granularity, the font size &amp; color is not fine tuned yet. The actual design system used in mockup is defined here: <https://www.figma.com/file/3tGoYDSrqU8Rqmstyn9NMn/Cofacts-UI-Design-Kit?node-id=0%3A5005|https://www.figma.com/file/3tGoYDSrqU8Rqmstyn9NMn/Cofacts-UI-Design-Kit?node-id=0%3A5005> We can go for Material UI's default first, and see if we need any adjustment in the future.

Comment on #305 RSS UI

he figma link provided by <https://github.com/LucienLee|@LucienLee> is at wireframe granularity, the font size &amp; color is not fine tuned yet. The actual design system used in mockup is defined here: <https://www.figma.com/file/3tGoYDSrqU8Rqmstyn9NMn/Cofacts-UI-Design-Kit?node-id=0%3A5005|https://www.figma.com/file/3tGoYDSrqU8Rqmstyn9NMn/Cofacts-UI-Design-Kit?node-id=0%3A5005> I suggest we use `&lt;Typography variant="caption"&gt;` for all `&lt;CustomTypography variant="subtitle2"&gt;`, and use `&lt;Typography variant="body2"&gt;` for all `&lt;CustomTypography variant="subtitle1"&gt;`.

Review on #305 RSS UI

The UI is beautiful &lt;3 thanks a lot! I have some suggestions regarding using the use of `CustomTypography` and code dependencies.

Comment on #305 RSS UI

We may need to add `copy-to-clipboard` to `package.json`, because it's currently a dev dependency. It won't be installed in production environment. I think `copy-to-clipboard`'s API is much simpler than the ClipboardJS we are currently using. Please consider removing `"clipboard": "^2.0.4"` and replace its usage (only in `CopyButton`) with copy-to-clipboard.

DSC-TW/SC-cofacts-extension

ichieh 2020-08-08 13:40:08

@5438arthur

ichieh 2020-08-08 13:42:43

@alanhc.tseng1999

lucien 2020-08-08 16:47:28

好讚！

alanhc 2020-08-08 20:53:34

哈囉這是我們之前串cofact api寫的請多多指教XD

lucien 2020-08-09 19:53:40

@5438arthur @alanhc.tseng1999 很高興你們參加 google challenge 使用我們的服務來擴展，好奇比賽結果什麼時候會出來呢？以及你們之後對這個插件有什麼想法，想要整理之後，一起放進 cofacts 大宇宙嗎？

alanhc 2020-08-10 13:41:00

原本規劃之後整理一下上架Chrome web store，上架前跟你們討論看看想法😆 我覺得可以整理一下放進cofacts大宇宙

lucien 2020-08-12 21:13:52

剛好我們最近介面改版中，我覺得介面可以討論改成相同樣式，然後一起放進我們組織的 repo

5

Review on #304 Fix 262 rss function

LGTM! It's beautiful. Thanks a million!

#262 Fix RSS function

#304 Fix 262 rss function

Review on #188 script to fetch google analytics stats

LGTM now! <https://github.com/ztsai|@ztsai> would you like to add <https://github.com/cofacts/rumors-api/pull/188#discussion_r465199034|stored script mechanism> in this PR or do you want to do it in another PR as an enhancement item?

1

Review on #305 RSS UI

Most of the PR looks good! Some final comments on `&lt;IFTTTItem&gt;` here.

Comment on #305 RSS UI

It's not recommended to use `&lt;table&gt;` for something that is not a data table. I suggest using flexbox for this particular UI, which should yield much simpler DOM and CSS style.

Comment on #305 RSS UI

If we use <https://material-ui.com/components/dialogs/|`Dialog`> instead of its underlying component <https://material-ui.com/components/modal/|`Modal`>, we would not need to define paper style and positioning here. Dialogs are not necessary used with `DialogTitle`, `DialogContent` etc, it can be used alone, as shown by `ListPageControls`' `Filters`.

g0v.hackmd.io

關於 / 教學 - HackMD

bil 2020-08-09 19:59:45

什麼條款呢？@@

lucien 2020-08-09 20:00:45

智慧財產權條款跟免責條款

bil 2020-08-09 20:57:10

放在about外吧、一般網站是不是都平行的放@@

bil 2020-08-09 21:11:43

更正，放在about內，但跟其他的介紹平行@@”

lucien 2020-08-09 21:11:58

了解

#311 ArticleReplyFeedbackControl revamp

This PR is a revamp to this component, including its popover: <https://user-images.githubusercontent.com/108608/89732120-487bdc00-da7f-11ea-9af3-06cdb1f5c13e.png|image> • Rename `ReplyFeedback` to `ArticleReplyFeedbackControl`.

#312 Upgrade to Apollo Client 3

Apollo Client 3 stable is <https://www.apollographql.com/blog/announcing-the-release-of-apollo-client-3-0/|released>. This PR <https://www.apollographql.com/docs/react/migrating/apollo-client-3-migration/|migrates Apollo Client to 3> and <https://github.com/adamsoffer/next-apollo/blob/master/CHANGELOG.md|next-apollo to latest>. `dataIdFromObject` we are currently using is deprecated in Apollo Client 3; however, in this PR I leave this intact because there are already too much code change.

Comment on #312 Upgrade to Apollo Client 3

Leaving out `{ssr: true}` in `withApollo` here will cause next.js's `useRouter` returning `{}` for `router.query` on server-side rendering &amp; first render on client, because next.js will think the page <https://nextjs.org/docs/api-reference/next/router#router-object|does not meet data fetching requirements>. It's valid for search page because this page is not meant to be accessed directly nor by search engines.

lucien 2020-08-10 14:50:29

我們公司上週有升，但似乎遇到一些 bug ，我建議再等個一兩週

lucien 2020-08-10 14:53:44

像是這個 https://github.com/apollographql/apollo-client/issues/6796

mrorz 2020-08-10 15:45:16

想說都已經 `3.1.3` 惹應該可以跳 QQ

lucien 2020-08-10 19:04:36

不不不，他今天又跳到 3.2 了

lucien 2020-08-10 19:04:42

瘋狂修 bug 中

mrorz 2020-08-10 19:16:14

我覺得 PR 還是可以先看起來
本週萌典松以前我應該還會在他後面蓋兩到三個 PR 把 article list page 給拔掉

mrorz 2020-08-10 19:16:33

其中一個是 https://github.com/cofacts/rumors-site/pull/311

mrorz 2020-08-10 19:18:21

可能最後 merge 完再補一個升級 Apollo Client 3.1 --> 3.2 之類

mrorz 2020-08-10 19:19:03

（沒遇到雷好像也不用 3.1 --> 3.2⋯⋯ XD）

mrorz 2020-08-10 19:21:16

https://github.com/apollographql/apollo-client/pull/6774 看起來 3.2 還要一陣子

他們還要把 master branch 換成 main branch 呢

Review on #311 ArticleReplyFeedbackControl revamp

#311 ArticleReplyFeedbackControl revamp

Comment on #188 script to fetch google analytics stats

Thanks! I'll add that in a separate PR as discussed offline

#181 Integrate with google analytics reporting API

#188 script to fetch google analytics stats

#189 Date util

#194 Load script

follow up PR for <https://github.com/cofacts/rumors-api/pull/188#discussion_r465199034|stored script mechanism> as discussed in <https://github.com/cofacts/rumors-api/pull/188|#188>

Comment on #194 Load script

<https://coveralls.io/builds/32649138|Coverage Status> Coverage increased (+0.5%) to 88.065% when pulling *<https://github.com/cofacts/rumors-api/commit/d2c3cd52b7a7cf5759f6ddcfbed3505328396c9c|d2c3cd5> on loadScript* into *<https://github.com/cofacts/rumors-api/commit/2ee6cc9cc8c654b25e2eaae2aa7b7558a7f1d9ee|2ee6cc9> on master*.

Comment on #126 Update all dependencies

*:warning: Artifact update problem* Renovate failed to update an artifact related to this branch. You probably do not want to merge this PR as-is. :recycle: Renovate will retry this branch, including artifacts, only when one of the following happens: • any of the package files in this branch needs updating, or • the branch becomes conflicted, or • you check the rebase/retry checkbox if found above, or • you rename this PR's title to start with "rebase!" to trigger it manually The artifact failure details are included below: *File name: package-lock.json*

Review on #190 Analytics data loader

LGTM! Thanks for the code change :rocket: