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 year and month select component #1385

Merged
merged 4 commits into from
Aug 28, 2023
Merged

Feat year and month select component #1385

merged 4 commits into from
Aug 28, 2023

Conversation

takurinton
Copy link
Contributor

@takurinton takurinton commented Aug 23, 2023

#975 (comment)
https://www.figma.com/file/fUeoFzCzL4rwRuW7CFW4m0/INGRED-UI?type=design&node-id=1263-1291&mode=design&t=0fnBZlfz2dyPtzgG-0

  • 年月を選択するコンポーネントの追加
    • 年月を選択できるので YearMonths という名前にしてる
  • カレンダーの部分のコンポーネントの分離
    • ref がうまく読めなくなるのでコンポーネントごと分割した
    • 単一選択はInnerCalendar
    • 範囲選択はInnerCalendarRange
    • 移植してるだけなので、styled.tsCalendar ディレクトリの下のものを共通で使う

@changeset-bot
Copy link

changeset-bot bot commented Aug 23, 2023

🦋 Changeset detected

Latest commit: bb96a59

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
ingred-ui Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@netlify
Copy link

netlify bot commented Aug 23, 2023

Deploy Preview for ingred-ui ready!

Name Link
🔨 Latest commit bb96a59
🔍 Latest deploy log https://app.netlify.com/sites/ingred-ui/deploys/64ec48796c649f0008b2a3f2
😎 Deploy Preview https://deploy-preview-1385--ingred-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@takurinton
Copy link
Contributor Author

アニメーションとかつけてないけど(今の構成だとつけられない?)けど、一旦挙動は満たしてる。

@takurinton
Copy link
Contributor Author

Comment on lines 72 to 79
<InnerCalendar
date={date}
current={current}
yearIsOpen={yearIsOpen}
isOutsideRange={isOutsideRange}
onYearIsOpen={setYearIsOpen}
onDateChange={onDateChange}
/>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

元々 <Calendar /> コンポーネントで読んでいたカレンダーは <InnerCalendar /> に分割。
分割しないと年月選択のコンポーネントの開閉で状態がリセットされないので observer の部分が呼ばれないため。

}
}, [current]);

useEffect(() => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

基本的に useScrollCalendar と同じ構成になっている。

@takurinton
Copy link
Contributor Author

takurinton commented Aug 23, 2023

TODO

  • style の修正
  • CalendarRange にも対応する
  • コンポーネントの整理(<InnerCalendar /> の追加に伴うもの)

Comment on lines +29 to +31
grid-auto-rows: ${({ theme }) => theme.spacing * 4}px;
grid-gap: ${({ theme }) => theme.spacing * 1.2}px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

かなり目分量

Copy link
Contributor Author

Choose a reason for hiding this comment

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

まあひとまず動かすところまでという目的に対してなのでこれはこれで良さそう。
style 修正はまとめて最後にやる予定なのでこれで問題ない。

@takurinton takurinton marked this pull request as ready for review August 28, 2023 03:18
@Yutan0423
Copy link
Contributor

バグばかり見つけてしまい恐縮ですが🙏
https://github.com/voyagegroup/ingred-ui/assets/71750401/3349bf45-444c-4b37-ab79-7d49aeb2a7da

Copy link
Contributor

@Yutan0423 Yutan0423 left a comment

Choose a reason for hiding this comment

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

ソースコード上で気になるところなしでした!
バグの部分、いつ直すのかみたいなところは判断していだだけるとmm

@takurinton
Copy link
Contributor Author

@Yutan0423
あんまりスクロールバーを引っ張って使うことを想定していないのでエッジケースかなとか思いましたがどう思いますか?(スクロールバーを非表示にする等の対応は取れそうかなとかは思いますが)
また、これはカレンダー部分でも同様なことが起きると思っています。

@Yutan0423
Copy link
Contributor

@takurinton
動画はドラッグですが、マウスのスクロールでも再現しています(大きく上にスクロールすると再現する)。
ですがそこまで頻繁に起こるわけでもなさそうなので、一旦は気にしなくても良いかなとは思います。
もし自分以外の方からも意見上がるようであれば、修正を検討いただけると!

@takurinton
Copy link
Contributor Author

@Yutan0423
なるほど
取れる対策としてパッと思いつくのは、ここで読み込む年数を増やす(今だと5年分だけど、これを10年分にする)とかですかね。
https://github.com/voyagegroup/ingred-ui/pull/1385/files#diff-9efe80a2ecd27d57ca14f62e020293a16c3b6f5f3490bd9275a7736bd283f2a5R8-R12

もし自分以外の方からも意見上がるようであれば、修正を検討いただけると!

そうですね、実運用で弊害が出るようであれば検討します、ありがとうございます!!!!

@takurinton takurinton merged commit 531572b into master Aug 28, 2023
2 checks passed
@takurinton takurinton deleted the feat-year branch August 28, 2023 23:14
@FluctMember FluctMember mentioned this pull request Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants