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

Events page #140

Merged
merged 29 commits into from
Jan 20, 2024
Merged

Events page #140

merged 29 commits into from
Jan 20, 2024

Conversation

Pugma
Copy link
Collaborator

@Pugma Pugma commented Dec 3, 2023

close #6

@Pugma Pugma added the page label Dec 3, 2023
@Pugma Pugma marked this pull request as ready for review December 5, 2023 10:27
@Pugma Pugma requested a review from mehm8128 December 5, 2023 10:28
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ありがとうございます!
まだある気もしてますが一旦ここらへん確認お願いします:pray:

src/pages/Events.vue Outdated Show resolved Hide resolved
src/pages/Events.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
@mehm8128
Copy link
Contributor

mehm8128 commented Dec 9, 2023

@Pugma (一応)再レビューしてよさそうなタイミングになったら再レビュー依頼お願いします:pray:(ハッカソン期間なのですぐにできるか微妙ですが)
image

@Pugma Pugma requested a review from mehm8128 December 27, 2023 08:40
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:

また、前回のやつ1つ見逃してそうな気するので確認してみてください:eyes:
#140 (comment)

src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Show resolved Hide resolved
src/components/UI/EventTypeAccordion.vue Outdated Show resolved Hide resolved
src/pages/Events.vue Outdated Show resolved Hide resolved
@mehm8128
Copy link
Contributor

mehm8128 commented Jan 8, 2024

これってサーバーの変更が必要な箇所以外もうレビューしてOKな状態だったりするんでしたっけ?

@Pugma
Copy link
Collaborator Author

Pugma commented Jan 8, 2024

そうですね、EventLevel以外は修正箇所をすべて反映してあるのでチェックしていただける状況です
完全にできたってわけじゃないのでちょっと再レビュー依頼は見送ってました
確認よろしくお願いします!

@mehm8128
Copy link
Contributor

mehm8128 commented Jan 8, 2024

OKです、じゃあそこ以外レビューしておきますー

src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
Comment on lines 10 to 23
enum All {
All = 3
}

type EventLevelWithAll = EventLevel | All

interface Props {
modelValue: EventLevelWithAll
}
const props = defineProps<Props>()

const emit = defineEmits<{
(e: 'update:modelValue', value: EventLevelWithAll): void
}>()
Copy link
Contributor

Choose a reason for hiding this comment

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

should
入出力(props, emit)をどっちもeventLevel.tsにあるEventLevelValueallを追加したunion型で行うようにしてほしいです(tsでenumをそのまま扱うことってあんまりない気がするので)

src/components/Events/EventItem.vue Outdated Show resolved Hide resolved
@Pugma Pugma requested a review from mehm8128 January 19, 2024 15:39
@Pugma Pugma linked an issue Jan 20, 2024 that may be closed by this pull request
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.

よさそうです!ありがとうございました

@Pugma Pugma merged commit a791a27 into main Jan 20, 2024
5 checks passed
@Pugma Pugma deleted the eventsPage branch January 20, 2024 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EventPageでのEventLevel取得方法の変更 /events
3 participants