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

feat: edit group reply #426

Merged
merged 15 commits into from
Mar 24, 2023
Merged

feat: edit group reply #426

merged 15 commits into from
Mar 24, 2023

Conversation

y-young
Copy link
Contributor

@y-young y-young commented Feb 26, 2023

Tracking: #112

@mergify mergify bot added the waiting-approval waiting code reviewers to approval PR label Feb 26, 2023
@github-actions
Copy link
Contributor

github-actions bot commented Feb 26, 2023

@codecov
Copy link

codecov bot commented Feb 26, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (52c5590) 83.09% compared to head (15f037b) 83.09%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #426   +/-   ##
=======================================
  Coverage   83.09%   83.09%           
=======================================
  Files          93       93           
  Lines        5388     5390    +2     
  Branches      427      426    -1     
=======================================
+ Hits         4477     4479    +2     
  Misses        904      904           
  Partials        7        7           
Impacted Files Coverage Δ
packages/design/components/Topic/Comment.tsx 92.24% <100.00%> (+0.06%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@trim21
Copy link
Contributor

trim21 commented Feb 26, 2023

https://pr-426-sites--bangumi-next.netlify.app/group/reply/2177421/edit 试了一下发现能用来修改帖子 b38

API的问题,修好了(

@mergify mergify bot added ci:test:fail and removed waiting-approval waiting code reviewers to approval PR labels Feb 27, 2023
@mergify mergify bot added waiting-approval waiting code reviewers to approval PR and removed ci:test:fail labels Feb 27, 2023
@trim21
Copy link
Contributor

trim21 commented Feb 27, 2023

感觉可以改一下 API,直接返回对应的帖子标题,前端就不需要请求两个 API 了

@mergify
Copy link

mergify bot commented Mar 1, 2023

@y-young this pull request is now in conflict 😩

@mergify mergify bot added the conflict label Mar 1, 2023
@mergify mergify bot removed the conflict label Mar 1, 2023
@mergify
Copy link

mergify bot commented Mar 3, 2023

@y-young this pull request is now in conflict 😩

@mergify mergify bot added the conflict label Mar 3, 2023
@mergify mergify bot removed the conflict label Mar 3, 2023
@trim21 trim21 added this to the v0.0.1 milestone Mar 3, 2023
Copy link
Contributor

@trim21 trim21 left a comment

Choose a reason for hiding this comment

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

等着改一下API的相应,没必要额外请求一次帖子内容。

@y-young
Copy link
Contributor Author

y-young commented Mar 7, 2023

才发现 API 3 天前就改好了(bgm38)

然而现在有个问题,如果用 useGroupTopic 的话必定会请求一次帖子数据,如果不用 useGroupTopic 的话似乎没法让主题里的回复内容在返回之前刷新,试了全局的 mutate 和 useSWRMutation 貌似都不行,刚返回的时候先看到还是旧数据。

@trim21
Copy link
Contributor

trim21 commented Mar 7, 2023

才发现 API 3 天前就改好了(bgm38)

然而现在有个问题,如果用 useGroupTopic 的话必定会请求一次帖子数据,如果不用 useGroupTopic 的话似乎没法让主题里的回复内容在返回之前刷新,试了全局的 mutate 和 useSWRMutation 貌似都不行,刚返回的时候先看到还是旧数据。

没有fetch过数据的话直接转跳回去不就能自动fetch新数据了吗?SWR应该是这样的吧?

@y-young
Copy link
Contributor Author

y-young commented Mar 7, 2023

没有fetch过数据的话直接转跳回去不就能自动fetch新数据了吗?SWR应该是这样的吧?

是这样,只不过会看到旧数据变成新数据的过程

@trim21
Copy link
Contributor

trim21 commented Mar 7, 2023

没有fetch过数据的话直接转跳回去不就能自动fetch新数据了吗?SWR应该是这样的吧?

是这样,只不过会看到旧数据变成新数据的过程

useGroupTopic 放在提交成功之后?

@y-young
Copy link
Contributor Author

y-young commented Mar 8, 2023

感觉看到旧数据更新到新数据的过程其实问题不大,可以加个 loading mask 解决。

那岂不是得在整个 topic 上加?

@FoundTheWOUT
Copy link
Contributor

在最后一行加这样?可能得问问设计师(

Avoid a topic request by not using useGroupTopic

Use global mutate to update reply text immediately,
revalidate in background
@y-young
Copy link
Contributor Author

y-young commented Mar 8, 2023

应该搞定了,记录一下过程:

如果使用 useSWRMutation 可以避免请求 topic,但昨天测试时无法及时更新 UI,把全局的 cache 打出来后发现是 cache 没有更新,原因是 populateCache 参数默认为 false,改成 true 就可以了

然而使用 useSWRMutation 还有个问题,在编辑回复页 mutate 时会请求一次帖子数据,在跳转回帖子时还会再请求一次,理论上时间相近的两次请求应该会去重,但不知道这里为什么没有

所以现在的方法是用全局的 mutate(避免调用 useGroupTopic),data 参数可以接收一个 mutate 函数,可以对帖子内容进行乐观更新,这样就省去了一次请求,在跳转回帖子后会在后台 revalidate,有问题会 rollback(如果这个请求不需要的话应该也可以去掉)

此外发现 mutate reply 也会发一次请求,现在把 revalidate 改成 false 就不会了

@mergify mergify bot added ci:build:fail and removed waiting-approval waiting code reviewers to approval PR labels Mar 9, 2023
@mergify mergify bot added waiting-approval waiting code reviewers to approval PR and removed ci:build:fail labels Mar 9, 2023
@trim21
Copy link
Contributor

trim21 commented Mar 9, 2023

等先合了另一个PR吧,改成error toast

trim21
trim21 previously approved these changes Mar 9, 2023
@trim21
Copy link
Contributor

trim21 commented Mar 11, 2023

parseInt('12abc') === 12

@trim21 trim21 modified the milestones: 0.0.0-alpha.0, next-next, next Mar 19, 2023
@y-young y-young requested a review from Ayase-252 March 24, 2023 08:28
@mergify mergify bot merged commit facb65f into bangumi:master Mar 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M waiting-approval waiting code reviewers to approval PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants