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

construct header section in detail view #57

Merged
merged 10 commits into from
Jun 7, 2021
Merged

Conversation

deep-diver
Copy link
Member

No description provided.

@codecov
Copy link

codecov bot commented Jun 6, 2021

Codecov Report

Merging #57 (5b323e3) into main (707eeec) will decrease coverage by 11.29%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##             main      #57       +/-   ##
===========================================
- Coverage   88.70%   77.41%   -11.30%     
===========================================
  Files           3        3               
  Lines          62       31       -31     
===========================================
- Hits           55       24       -31     
  Misses          6        6               
  Partials        1        1               
Impacted Files Coverage Δ
server/pkg/serv/serv.go 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c3c6fca...5b323e3. Read the comment docs.

Copy link
Member

@kkweon kkweon left a comment

Choose a reason for hiding this comment

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

아주 좋습니다 👏👏👏👏👏

몇가지 사항 및 린트에러만 수정되면 문제 없겠습네다.

client/lib/screens/detail_screen.dart Outdated Show resolved Hide resolved
@@ -0,0 +1,10 @@
import 'package:flutter/material.dart';
Copy link
Member

Choose a reason for hiding this comment

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

실수로 들어간 파일 같네요

Copy link
Member Author

Choose a reason for hiding this comment

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

현재 디테일뷰 섹션 분리를

  1. 헤더 (시청 수, 좋아요 수, 발표자 등)
  2. 논문 abstract 열람
  3. 코드 저장소 목록
  4. 추천 논문 목록

으로 채울 생각입니다. abstract.dart는 이 중 두 번째 위젯에 해당합니당

Copy link
Member

Choose a reason for hiding this comment

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

abstract 이 키워드라서 PaperAbstract 라고 명시해주시면 좋을 것 같아요.
나중에 보시면 수정

Comment on lines 5 to 9
String presenter = "박찬성";
int numberOfViews = 0;
int numberOfLikes = 0;
bool didILikedIt = false;
String date = "2017.4.22";
Copy link
Member

Choose a reason for hiding this comment

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

하드코딩보다 이미 가지고 있는 도메인 (Video 객체)로 설계해주세요.
아니면 VideoWrapper 데이터 객체를 생성하거나요.

Copy link
Member Author

Choose a reason for hiding this comment

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

이거는 일부 데이터를 Video 객체를 참조하여 대체하였습니다.
다만 numberOfViews, didILikedIt, date 에 대한 정보는 형재 Video에 정의되어 있지 않아서 플레이스 홀더로 남겨두었습니다.

혹시 관련 정보도 같이 프로토버퍼 메시지에 추가한 뒤 다시 PR 드릴까요?

  • 근데 didILikedIt 은 사용자 관련 정보여서, 요놈은 아직 정의하기 애매할것도 같네요.

Copy link
Member Author

Choose a reason for hiding this comment

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

프로토 메시지도 일부 수정하여 업로드 하였습니다.
코드 커버리지는 왜 fail 인지 잘 모르겠네요 ㅜ

client/lib/widgets/detail/header.dart Outdated Show resolved Hide resolved
client/lib/widgets/detail/youtube.dart Outdated Show resolved Hide resolved
Copy link
Member

@kkweon kkweon left a comment

Choose a reason for hiding this comment

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

수고하셨습니다.

@@ -0,0 +1,10 @@
import 'package:flutter/material.dart';
Copy link
Member

Choose a reason for hiding this comment

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

abstract 이 키워드라서 PaperAbstract 라고 명시해주시면 좋을 것 같아요.
나중에 보시면 수정

@kkweon
Copy link
Member

kkweon commented Jun 7, 2021

커버리지는 리베이스 최신화 안하셔서 그렇습니다.

@kkweon kkweon enabled auto-merge (squash) June 7, 2021 17:23
@kkweon kkweon disabled auto-merge June 7, 2021 17:23
@kkweon kkweon merged commit bbba5e8 into main Jun 7, 2021
@kkweon kkweon deleted the header-section-in-detailview branch June 7, 2021 17:24
@kkweon
Copy link
Member

kkweon commented Jun 7, 2021

수고하셨습니다.

@github-actions
Copy link

github-actions bot commented Jun 8, 2021

🎉 This PR is included in version 1.0.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

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.

2 participants