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: group new topic page #320

Merged
merged 18 commits into from
Feb 13, 2023
Merged

Conversation

y-young
Copy link
Contributor

@y-young y-young commented Jan 29, 2023

close #235

http://localhost:5173/group/sandbox/new_topic
https://pr-320-sites--bangumi-next.netlify.app/group/sandbox/new_topic

  • 由于发帖页面的没有继承小组页面的通用布局,因此调整了一下目录结构,将原来 /group/[name] 里的页面移到了 /group/[name]/index 目录里
  • 加入小组的按钮应该是一个通用的按钮类型,design 包里暂未实现,且目前的封装较为混乱,不少地方重复用了自定义样式,打算另开 PR 整理一下,正在同设计师讨论
  • 合并所有已拆分的前置 PR

@github-actions
Copy link
Contributor

github-actions bot commented Jan 29, 2023

@codecov
Copy link

codecov bot commented Jan 29, 2023

Codecov Report

Base: 88.81% // Head: 88.70% // Decreases project coverage by -0.12% ⚠️

Coverage data is based on head (617fa12) compared to base (491430e).
Patch coverage: 89.92% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #320      +/-   ##
==========================================
- Coverage   88.81%   88.70%   -0.12%     
==========================================
  Files          86       88       +2     
  Lines        4874     5019     +145     
  Branches      517      529      +12     
==========================================
+ Hits         4329     4452     +123     
- Misses        545      567      +22     
Impacted Files Coverage Δ
...ges/index/group/[name]/components/NewTopicForm.tsx 78.43% <78.43%> (ø)
...bsite/src/pages/index/group/[name]/index/index.tsx 93.75% <93.75%> (ø)
...ges/website/src/pages/index/group/[name]/index.tsx 100.00% <100.00%> (+6.45%) ⬆️
...ite/src/pages/index/group/[name]/index/members.tsx 96.20% <100.00%> (ø)
...e/src/pages/index/group/components/GroupLayout.tsx 100.00% <100.00%> (ø)
...c/pages/index/group/components/GroupNavigation.tsx 100.00% <100.00%> (ø)

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 at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

So that react-hook-form can focus the field when there's an error.
To ease the use with react-hook-form
@trim21 trim21 mentioned this pull request Jan 29, 2023
2 tasks
@trim21
Copy link
Contributor

trim21 commented Jan 29, 2023

如果觉得合并进来不应该是一个commit的话直接分成几个PR比较好吧。

登录的问题等着跟sai老板商量一下sso...

@y-young
Copy link
Contributor Author

y-young commented Jan 29, 2023

如果觉得合并进来不应该是一个commit的话直接分成几个PR比较好吧。

主要这些都是跟这个 PR 关联的一些前置重构和修复,如果拆的话不就得多出三四个额外的 PR?

不明白一个 PR 产生多个 commit 有什么问题……

@Ayase-252
Copy link
Contributor

一个 PR 尽量只干一件事情,更小的 PR 会更好 review 一点

@y-young
Copy link
Contributor Author

y-young commented Jan 29, 2023

我的习惯是维护一个整洁的 commit 历史,然后就可以按 commit review。

那我看看再拆两个 PR 出来吧……

按我的理解每个 commit 都是做一件事,如果每个 PR 也只做一件事的话就得有 6 个 PR

@pull-request-size pull-request-size bot added size/L and removed size/XL labels Feb 1, 2023
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Feb 1, 2023
@y-young
Copy link
Contributor Author

y-young commented Feb 12, 2023

设计稿里把小组的右栏宽度改窄了,发帖按钮的样式也改了,这里先按新的做了,其他的等另外的 PR
image

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.

没检查用户是否登录?

@y-young
Copy link
Contributor Author

y-young commented Feb 13, 2023

没检查用户是否登录?

维基编辑页也没检查吧,等于这里也是交给后端做了,前端检查后面跟权限一起做吧?

@trim21
Copy link
Contributor

trim21 commented Feb 13, 2023

维基编辑页也没检查吧,等于这里也是交给后端做了,前端检查后面跟权限一起做吧?

那分成不同的pr后面再加上吧。

顺便做个某些路由下未登录时转跳登录页面,登录成功后再转跳回来的功能。

@y-young
Copy link
Contributor Author

y-young commented Feb 13, 2023

登录成功后再转跳回来的功能。

你提醒我了,这个之前打算做的b38

@trim21
Copy link
Contributor

trim21 commented Feb 13, 2023

功能看起来ok,代码等其他人review了

@trim21
Copy link
Contributor

trim21 commented Feb 13, 2023

还需要根据设计稿更新一下小组信息这个组件 🤔

@trim21 trim21 requested a review from a team February 13, 2023 06:29
@trim21
Copy link
Contributor

trim21 commented Feb 13, 2023

左右宽度跟设计稿不太一样,是要放在另外的PR?

@y-young
Copy link
Contributor Author

y-young commented Feb 13, 2023

左右宽度跟设计稿不太一样,是要放在另外的PR?

是,我上面说了,做的过程中设计稿改了,但是小组的左右栏布局和维基编辑页是一样的,设计稿里貌似还没改完

@FoundTheWOUT
Copy link
Contributor

image

@y-young
Copy link
Contributor Author

y-young commented Feb 13, 2023

image

你是说调整 textarea 大小吗?我觉得这个场景下布局基本都会被破坏掉,尤其现在边框是加在编辑器上而不是 textarea 上,反正我试了好几次都没有找到合适的解决方法,你们谁要是有兴趣可以试着修一修 🥲

@FoundTheWOUT
Copy link
Contributor

没有找到合适的解决方法

可以

  1. 不允许设置宽度
  2. 控制最小/最大宽度

@FoundTheWOUT
Copy link
Contributor


@cokemine 当初这里是为啥要 9999,现在会导致覆盖掉了Header,Header是5000

image

@y-young
Copy link
Contributor Author

y-young commented Feb 13, 2023

可以

  1. 不允许设置宽度
  2. 控制最小/最大宽度

另开 Issue 或 PR 讨论吧,这块坑挺多的,组件那里在写小组讨论页的时候把宽度写死了还没改
当然如果不允许调整宽度就没有这些问题了(

@FoundTheWOUT
Copy link
Contributor

另开 Issue 或 PR 讨论吧,这块坑挺多的,组件那里在写小组讨论页的时候把宽度写死了还没改 当然如果不允许调整宽度就没有这些问题了(

确实,感觉还涉及到 EditorForm 和 Form 的问题

@cokemine
Copy link
Contributor

应该是当时被其他的给盖住了。就直接设了9999,调成5000以内应该不会有啥问题

@cokemine
Copy link
Contributor

可以

  1. 不允许设置宽度
  2. 控制最小/最大宽度

另开 Issue 或 PR 讨论吧,这块坑挺多的,组件那里在写小组讨论页的时候把宽度写死了还没改 当然如果不允许调整宽度就没有这些问题了(

主要是为了契合原站逻辑,如果直接固定宽度的话这个应该还可以简化一些,为了可以自由调整宽度当时这个编辑框的写法也是挺奇怪的,也没有考虑到上面可能还一个框要跟着动(

@FoundTheWOUT FoundTheWOUT mentioned this pull request Feb 13, 2023
@mergify mergify bot merged commit 819a05e into bangumi:master Feb 13, 2023
@mergify mergify bot added waiting-opened-48h PR will be merged by bot after it's opened 48h and removed waiting-review labels Feb 13, 2023
@FoundTheWOUT
Copy link
Contributor

额。。merge 了,还想让 @Ayase-252 看下的(尴尬

@y-young y-young deleted the feat/group-new-topic branch February 14, 2023 06:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/XL waiting-opened-48h PR will be merged by bot after it's opened 48h
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tracking Issue: 发帖/回帖
5 participants