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.

Sign up for GitHub

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

Jump to bottom

Feat/event modal #82

Merged
merged 17 commits into from
Nov 13, 2023
Merged

Feat/event modal #82

merged 17 commits into from
Nov 13, 2023

Conversation

helgev-traP
Copy link
Contributor

@helgev-traP helgev-traP commented Sep 14, 2023 •
edited
Loading

close #52

./src/components/Events/EventModal.vue の中に作ってます。

propsでopenStatusを受け取ってupdate-public-statusを引数付きで発火します。

コミットの3,4つ目は間違えたやつです

interface Props {
  eventLevel: EventLevel
}

いったんenumのままにしてます

11/2 23:30
一個だけ直すので数時間お待ちください、、、
↑おわりました！

(11/10, 2:27)
レビューの点の修正 + gap, paddingもremにしました
不要なCSSを消しました

PropsでEventLevelとしてeventLevelを受け取ってupdate-public-statusを引数EventLevel付きで発火します

@helgev-traP helgev-traP marked this pull request as ready for review September 15, 2023 16:10
mehm8128
mehm8128 suggested changes Sep 16, 2023
View reviewed changes
Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

最初なのでちょっと多めですが、確認してほしいです！
分からないところとかあればまた聞いてください

mehm8128
mehm8128 reviewed Sep 16, 2023
View reviewed changes
Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

あ、あとPRの説明文にclose https://github.com/traPtitech/traPortfolio-Dashboard/issues/52とかって書くとissueを紐付けられるので、お願いしますー(PRをマージしたときに自動でissueもclose されるようになります)

@helgev-traP helgev-traP requested a review from mehm8128 November 2, 2023 13:58
mehm8128
mehm8128 reviewed Nov 2, 2023
View reviewed changes
Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

大体よさそうなのですが、細かいところ指摘しました
確認お願いします:pray:

@helgev-traP helgev-traP requested a review from mehm8128 November 9, 2023 17:36
mehm8128
mehm8128 reviewed Nov 10, 2023
View reviewed changes
Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

1箇所だけ:pray:

次回からでいいのですが、PRの説明文はPRの概要とか相談したい点とかを書く場所にしたいので、レビューのたびに修正内容とかを追加する必要はないです
修正内容はcommit messageに書いてもらってるので、そこで分かります:+1:

あと、しゅーまいさんが出してくれた修正とconflictしてる箇所があるのですが、大変そうなのであとで確認します
できそうなら直してもらいますが、難しそうなら僕が直しちゃうかもです

src/components/Events/EventLevelMenu.vue Outdated
@click="emit('update-public-status', level)"
>
<p :class="$style.statusName">{{ detail.label }}</p>
<p :class="$style.description" style="">
Copy link
Contributor

Choose a reason for hiding this comment

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

nits
style属性不要になったので完全に消しておいてほしいです

@helgev-traP
Copy link
Contributor Author

あ、消し忘れてたので消します。
conflictのやつは\n追加するだけなのでしゅーまいさんのやつを取ってきます
main -> Feat/event modalにマージしてこちらでconflictを解消するのでいいですか

@mehm8128
Copy link
Contributor

conflictはデータ構造変わったのでコンポーネントで使う型とかももしかしたら変えないといけないかもで、帰るまでちゃんと見れないので夕方までには結論出します:pray:

@SSlime-s
Copy link
Member

  1. Props と Emits で EventLevel を受け取るのではなく、EventLevelValue を受け取るようにする
    もしくは、Props で受け取った EventLevel を eventLevelValueMap と computed を使って、component 内で EventLevelValue にしたものを持つ (この場合は emit する際は getEventLevelFromValue で value -> level をする)
  2. v-for では v-for="[level, detail] in Object.entries(eventLevels) のように eventLevels を Object.entries で配列にしたものを使う
    (上記の例だと level には EventLevelValue が入ることになります)
  3. v-for 内の disabled の判定には 1. で EventLevelValue にしたものと level を比較する

と動くと思います

@mehm8128
Copy link
Contributor

↑らしいのですができそうですか？難しそうならさっきのstyle消すやつだけやってもらえればconflict解消は僕がcommit作ります

@helgev-traP
Copy link
Contributor Author

やってみます

@helgev-traP
Copy link
Contributor Author

やってみました
一応動いてるんですがもしかしたら遠回りしてるかもしれないです

mehm8128
mehm8128 reviewed Nov 11, 2023
View reviewed changes
Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

大体よさそうですがちょっとだけ:pray:

src/components/Events/EventLevelMenu.vue Outdated

<template>
<div :class="$style.eventLevelMenu">
<div v-for="[key, value] in Object.entries(eventLevels)" :key="key">
Copy link
Contributor

Choose a reason for hiding this comment

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

imo
keyとvalueじゃなくて変更前のような命名でよさそうです

src/components/Events/EventLevelMenu.vue Outdated
defineProps<Props>()

const emit = defineEmits<{
(e: 'update-public-status', value: EventLevelValue): void
Copy link
Contributor

Choose a reason for hiding this comment

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

imo
今更だけどここ、propsと合わせてupdate-event-levelの方がよさそうです

mehm8128
mehm8128 approved these changes Nov 12, 2023
View reviewed changes
Copy link
Contributor

@mehm8128 mehm8128 left a comment

Choose a reason for hiding this comment

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

ありがとうございます！
lintのエラー出てるのだけ直してもらえればマージしてOKです

@helgev-traP
Copy link
Contributor Author

これって自分でマージしちゃっていいんですか？

@mehm8128
Copy link
Contributor

OKです！

@helgev-traP helgev-traP merged commit b7a092b into main Nov 13, 2023
@helgev-traP helgev-traP deleted the feat/event_modal branch November 13, 2023 11:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SSlime-s SSlime-s SSlime-s left review comments

@mehm8128 mehm8128 mehm8128 approved these changes

Assignees

@helgev-traP helgev-traP

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

EventModalの作成
3 participants
@helgev-traP @mehm8128 @SSlime-s