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(mail): implement the send mail view #337

Merged
merged 23 commits into from
Oct 4, 2021

Conversation

Rok93
Copy link
Contributor

@Rok93 Rok93 commented Sep 16, 2021

작업 내용

  • 그룹 발송 기능
  • 개별 발송 기능

구현하지 못한 부분

  • 모든 평가 상태를 조회하는 경우
    GroupMailFormView#createEvaluationStatusItem 부분을 적절히 수정하면 될 것 같은데...
    이 부분은 하드코딩하는 방법 말고는 없을까요...? 🤔

사용법

1. 관리자 화면에서 메일 발송 탭 클릭

image

2. 그룹 발송 혹은 개별 발송 버튼 클릭

image

3. 기본적으로 메일 폼은 아래와 같다.

개별 발송그룹 발송수신자를 고르는 컴포넌트만 다르다! (화면이 잘리는데 하단에 전송 버튼 있음)

그룹 발송
그룹 발송의 경우 모집, 평가, 평가 상태를 클릭하면 해당 조건에 맞는 mailTargets들을 수신자로 추가한다.
image

개별 발송

  • 개별 발송의 경우 수신자를 직접 추가하거나 플랫폼의 지원자들을 keyword 검색(= email or 이름 의 일부)하여 검색하여 추가(= 지원자 검색 버튼으로 접근)할 수 있다.
    image

수신자를 추가하면 화면 상에 추가 된 수신자가 초록색 텍스트박스로 출력된다. 우측에 X 버튼을 클릭하면 해당 수신자는 수신자 리스트에서 제거할 수 있다. (그룹 발송, 개별 발송 모두 동일하게 적용)

image

동작 확인

이미지 업로드 기능

용량이 초과하면 이미지 파일을 업로드할 수 없다. (https://vaadin.com/docs/v14/flow/components/tutorial-flow-upload) 참고하여 setMaxFiles 메서드 사용할 것! (자바 기준, 코틀린은 필드로 접근하여 값을 직접 할당)
58101332-CF4B-4ED0-88FF-1C578F93A3C1
0C6FBEE0-2AB1-4C3F-9597-31E58D69C37D

용량만 초과하지 않는다면, 이미지 갯수는 상관없는 것 같다.
1D651303-79EA-4777-B6EC-389D579006CB

용량을 변경하고 싶다면… [Upload | Using Vaadin Components | Flow | Vaadin 14 Docs]
용량 제한 변경 방법 예시 (숫자는 아무 숫자나 입력한 것이니 참고만 할 것!)
9F15740B-7A61-433A-9196-2F2BE98DF87C

이미지 파일을 업로드했을 때…. 포맷

Http-Server 미션 때 배운내용을 활용하여… OutputStream을 생각하면 되는건가…?
Mail 전송할 때, 어떤 타입이 필요한지를 알아서, 최대한 그 타입으로 미리 만들면 좋을 것 같음.

MultiFileMemoryBuffer class에 대한 고찰

Buttons 파일에 ImageUpload Button을 구현하였고 코드는 아래와 같다.

fun createImageUpload(
    text: String,
    receiver: MultiFileMemoryBuffer,
    succeededListener: UploadSucceededListener
): Upload {
    return Upload(receiver).apply {
        setAcceptedFileTypes("image/jpeg", "image/png", "image/gif")
        uploadButton = createPrimaryButton(text) { }
        addSucceededListener {
            receiver.getInputStream(it.fileName)
        }
    }
}

Upload | Using Vaadin Components | Flow | Vaadin 14 Docs 참고

  • The MultiFileMemoryBuffer class is a ready-made implementation of MultiFileReceiver.
  • The data is stored in files on the file system and you can get the data using a file name and the getInputStream(String) method.

메일 용어 참고

What is a Message Body?

  • subject: 메일 제목
  • recipient: 수신자
  • body: 메일 본문
  • (MailFormView의 currentRecipients는 현재 선택된 수신자들을 Text Component로 가지는 HorizontalLayout )

Closes : #304

@Rok93 Rok93 self-assigned this Sep 16, 2021
Copy link
Contributor

@knae11 knae11 left a comment

Choose a reason for hiding this comment

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

상당히 많은 부분을 구현해주셨네요!

여러개 넣고 동작시켜보았어요. 사실 제가 바딘 작업을 거의 안 해서 잘 모르긴 하지만, 사용자라고 생각하고 몇가지 의견 남겨보았어요!ㅎㅎㅎ 혹시 작업 범위를 벗어난 작업이거나 어려운 작업을 요청드린 거라면 알려주세요!ㅎㅎㅎㅎㅎ

title.apply { setSizeFull() }
)

val receivers = VerticalLayout(H4("수신자"), createReceiverFilter())
Copy link
Contributor

Choose a reason for hiding this comment

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

수신자 목록이 모집, 평가, 모집상태 하부에 뜨는게 어떨까요?


val sendButton = VerticalLayout(
createPrimaryButton("전송") {
println("title: ${title.value} receivers: ${this.receivers} content: ${this.content.value}")
Copy link
Contributor

Choose a reason for hiding this comment

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

혹~시 제가 해당 print문을 못찾는 걸까요?ㅠㅠㅠ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

어.... 이 부분은 아직 emailService에 지금의 기능에 적합한(?) 메일 전송 기능이 구현되어 있지 않아 비워뒀습니다.
print문만 두면 오해의 여지가 있을 것 같아 현재는 todo로 표시해두었습니다.

혹시 전송 버튼을 눌러도 콘솔 창에 출력이 안된다는 말씀이신가요?? 🤔

Copy link
Contributor

Choose a reason for hiding this comment

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

네네ㅠ콘솔에 안 뜨는것 같아서 남겼는데 제가 못찾는거라면 알려주세요ㅎㅎㅎ

Comment on lines 61 to 63
currentRecipients.apply { setSizeFull() },
uploadFile,
mailBody,
Copy link
Contributor

Choose a reason for hiding this comment

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

VerticalLayout(currentRecipients.apply { setSizeFull() }),
VerticalLayout(uploadFile),

기존

image

수정후

image

이렇게 해주면 해당 라인이 맞아져요.ㅎㅎㅎ 이거 VerticalLayout이 들어가게 메소드로 빼서 예쁘게 코드 수정해 주셔도 좋을 듯 싶어요! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

현재 대대적인 개편작업 중인데, 때마침 이 부분은 배럴이 코멘트 남겨주신 것과 동일하게 변경했네요! 👍

val emailTarget = TextField().apply {
value = email
isReadOnly = true
style.set("background-color", "#00B493")
Copy link
Contributor

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.

아.... 배민 색을 따온건데.... 👀
다들 이 색상에 대한 궁금증을 가지시더라구요 😅
무난한 회색으로 변경하였습니다. 😃

protected val subject: TextField = TextField("메일 제목", "메일 제목 입력")
protected val recipients: MutableList<String> = mutableListOf()
protected val body: TextArea = createMailBody()
private val currentRecipients = HorizontalLayout()
Copy link
Contributor

Choose a reason for hiding this comment

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

image
ㅠㅠㅠㅠ 저 바딘 잘 몰라서 사실 잘 모르는데... 갯수가 많아지면 옆으로 계속 늘어날 것 같은데 이건 괜찮까 싶네요!
단체메일보내면 꽤 많을 듯 싶어서요.
더불어..x의 위치도 아래로 변경됩니당!

Copy link
Contributor

Choose a reason for hiding this comment

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

개별 조회에서 지원자 조회 탭을 누르지 않고 test 하고 엔터만 치면 그대로 입력되는데 이것도 괜찮을까요..? 해당하는 이메일을 불러와야 하지 않을까 싶어서요 혹은 엔터 칸을 없애야하지 않을까 싶기도 해요! 로키의 작업범위가 아니라면 알려주세요~ㅎㅎㅎ

Copy link
Contributor Author

@Rok93 Rok93 Sep 22, 2021

Choose a reason for hiding this comment

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

엔터 키를 누르거나 혹은 엔터 버튼을 눌렀을 때, 직접 작성한 이메일이 등록되는 기능은 이슈의 내용 중 일부인 아래의 요구사항 때문에 추가한 기능입니다.
현재 저희 지원 페이지에 가입되어있는 이메일이 아니더라도 메일을 보낼 수 있도록하기 위함입니다 😃
(물론 이메일양식에 맞지 않는 이메일을 등록한다거나... 등등 여러가지 예외 사항들을 잡으면 좋겠지만... 그 부분까지는 고려하지 못했네요 🥲)

요구사항 중 일부

  • 메일 주소를 직접 입력하여 특정한다.
    • 테스트 등을 위해 직접 메일주소를 입력하여 발송할 수 있어야 한다.
    • 발송할 메일 주소 입력 UI를 구현한다.

Copy link
Contributor Author

@Rok93 Rok93 Sep 22, 2021

Choose a reason for hiding this comment

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

배럴이 발견하신 문제점... x위치가 아래로 내려가는 문제는 currentRecipients를 Div 컴포넌트로 변경하니까 전보다는 나아진 것 같네요 🤔

변경 후 모습
image

this.currentRecipients.removeAll()
this.currentRecipients.apply {
recipients.forEach {
add(createRecipientComponent(it))
Copy link
Contributor

Choose a reason for hiding this comment

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

image
여러개를 추가할 때, 추가되어도 옅은 색으로만 바뀌어서 좀 헷갈리는 것 같아요. 추가가 되면 추가 -> 삭제로 변경되거나 색 대비를 더 주어도 좋을 것 같다는 생각이 들어요! 혹시 어려운 작업이라면 반영하지 않으셔도 좋습니다!ㅎㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분은 저도 변경해보고 싶었는데... component를 replace 하도록 해보려했는데... 생각한대로 잘 안돼서 지금처럼 Disable로 변경했는데... 음 일단은 지금 방식이 최선이었는데 이 부분은 혹시 추가 버튼 클릭 시에, 삭제 버튼(다른 버튼)으로 변경할 수 있는 방법 아시는 분...의 제보를 받습니다 ... 썸바딘 헬프 유...

@Rok93
Copy link
Contributor Author

Rok93 commented Sep 22, 2021

변경사항

피드백 받은 내용을 토대로 변경된 사항

개별 발송, 그룹 발송을 하나의 페이지로 구현

image

  • 불러오기 (=기존 개별 발송 기능), 그룹 불러오기 (=기존 그룹 발송 기능)
  • 각 기능들은 Dialog로 구현하였고, 각각 Dialog를 상속받은 컴포넌트 클래스를 만들었습니다. (MailFormView 클래스의 크기가 너무 커져서 분리하였습니다)

보낸사람 추가

image

  • 보낸 사람은 applicaion-properties의 email 주소로 고정됩니다. (local 환경에서는 EMAIL)

그룹 보내기 기능

  • 특정 지원, 평가, 평가 상태를 선택하면 아래에 조회된 mailTarget들을 표시한다.
  • 추가 버튼을 누르면, 받는사람에 추가된다.

image

추가 이후
image

불러오기 기능 (= 개별 발송)

  • keyword(= 이메일 or 이름) 입력하여 조회
  • 추가하고 싶은 사용자 추가 버튼 클릭
  • (이미 추가되어 있는 경우) 삭제하고 싶은 사용자는 삭제 버튼 클릭
  • 추가 혹은 삭제가 끝났다면 적용버튼 클릭하여
    image

적용 버튼 클릭 시 화면
image

업로드 버튼 본문 하단에 위치하도록 변경

image

그 외

  • 업로드 버튼이 모든 파일 타입을 업로드 가능하도록 허용
  • 업로드 최대 허용 용량을 10MB로 제한 (이전에는 default 값)

@Rok93
Copy link
Contributor Author

Rok93 commented Sep 23, 2021

변경사항

  • 받는 사람을 Grid로 표현하여 스크롤 할 수 있게 변경
  • 추가 된 이메일은 그리드 창의 삭제버튼을 통해서만 삭제할 수 있다.
  • mailTargets 필드를 MutableSet 컬렉션으로 둬서 중복 요소 제거 (중복 추가X)
  • MailFormViewBindingFormLayout를 상속받도록 변경
  • EmailTarget 정보로 이름 필드를 추가 (기존에는 email 요소만 존재)
  • 보낸사람 정보는 MailProperties로 가지고와서 지정하도록 변경
    • 기존 방식: properties의 정보를 @Value로 읽어오도록 했음

image

업로드 버튼의 사용방법

  • Service 기능이 따로 없다보니 테스트를 못해봄..
  • upload 객체를 잘 이용하면 될 것 같음
  • Upload 클래스의 getOutputStream()을 통해 outputStream을 이용하면 될 것 같다.

image

추가 변경 점

  • (라벨 변경)

image

Copy link
Contributor

@NewWisdom NewWisdom left a comment

Choose a reason for hiding this comment

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

로키 !!!
메일 발송 화면 구현하느라 진짜진짜진짜 수고 많았습니다 ㅠㅠㅠ
밑바닥서부터 구현해야해서 저였어도 정말 막막했을 것 같네요 🥲
어제 이후로 작업 하신 부분 확인했는데 진짜 많이 개선이 되었네요 노고가 보입니다 짱짱 👏
보면서 저도 로컬에서 코드를 좀 고쳐보고 도움됐음 좋겠어서
일단 먼저 지금 상황에서의 제 의견을 좀 남겨보았는데,
이게 제 개인적인 의견이고 좋은 방향인지에 대한 명확한 확신이 없으니
오히려 방해가 될 수 있을 것 같다는 생각에 조금 조심스럽네요.
제가 남긴 의견들은 읽어보시고 반영하지 않아도 되고 무시하셔도 매우 좋습니다 !!
그냥 참고만 부탁드려요 !!
혹시 제 코멘트가 이상하거나 도움이 필요하면 편하게 말씀해주세요 ㅎㅎㅎ
화이팅 🔥

Comment on lines 79 to 92
return VerticalLayout(
subjectText,
sender,
recipientFilter,
VerticalLayout(mailTargetGrid),
mailBody,
VerticalLayout(uploadFile),
sendButton
).apply {
setSizeFull()
justifyContentMode = FlexComponent.JustifyContentMode.CENTER
defaultHorizontalComponentAlignment = FlexComponent.Alignment.CENTER
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return VerticalLayout(
subjectText,
sender,
recipientFilter,
VerticalLayout(mailTargetGrid),
mailBody,
VerticalLayout(uploadFile),
sendButton
).apply {
setSizeFull()
justifyContentMode = FlexComponent.JustifyContentMode.CENTER
defaultHorizontalComponentAlignment = FlexComponent.Alignment.CENTER
}
}
return VerticalLayout(
subject,
sender,
recipientFilter,
mailTargetGrid,
content,
uploadFile
).apply {
setSizeFull()
justifyContentMode = FlexComponent.JustifyContentMode.START
defaultHorizontalComponentAlignment = FlexComponent.Alignment.START
}

조금 실험을 해보았는데 각 요소의 VerticalLayout을 빼고, createMailSendButton()도 따로 분리한 후
START 로 정렬을 하면 기존과 같은 정렬이 유지되네요 ! (제안한 코드가 그대로 복붙은 안될거에요)
그러면 각 요소간 공백도 좀 줄어 보기 더 깔끔한 것 같다는 생각도 들어요.
좀 더 개선할 방법이 없을까 고민하다가 제안하는 거라 로키 의견대로 참고만 부탁드려요!!
image

Copy link
Contributor

Choose a reason for hiding this comment

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

저도 마찌의 의견에 공감해요!
굳이 모든 요소들을 컴포넌트로 한번더 감쌀것 없이 바로 VerticalLayout을 반환하도록 한다면 훨씬 깔끔한 코드가 될 것 같네요 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

저도 불필요하게 감쌀 필요 없다고 생각해요!
마찌가 제안해준 코드에서 조금 더 수정해보면서 변경해볼게요!! 🙏

Comment on lines 62 to 66
val subjectText = VerticalLayout(H3("메일 제목"), subject).apply { setWidthFull() }
val sender = VerticalLayout(H4("보낸사람"), createSender())
val recipientFilter = VerticalLayout(H4("받는사람"), createRecipientFilter())
val mailBody = VerticalLayout(content)
val uploadFile = createUploadButton("첨부파일", MultiFileMemoryBuffer()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

VerticalLayout을 제거하면 여기도 많이 줄일 수 있을 것 같아요 :)

private fun createAddRecipients(): Component {
val container = VerticalLayout()
return VerticalLayout(
createSearchBar {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
createSearchBar {
createSearchBar("지원자 검색") {

여기다 다음과 같은 라벨을 주어도 좋을 것 같네요 ㅎㅎ

Comment on lines 47 to 49
createErrorButton("취소") {
close()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
createErrorButton("취소") {
close()
}
createContrastButton("취소") {
close()
}

다른 곳과 같이 취소에는 ContrastButton을 사용하면 좋을 것 같아요!

Comment on lines 23 to 30
init {
width = "800px"
height = "90%"
add(H2("지원자 정보 조회"), HorizontalLayout(createAddRecipients())).apply {
setWidthFull()
}
open()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

image

다이얼로그를 쓰는데 화면이 꽉찬 느낌이기도 하고,
들어가는 데이터들의 너비 사이즈(?)도 많지 않아서사이즈를 조금 줄여도 좋을 것 같은데 어떠신가요??

그리고 현재는 빈 화면을 클릭해야 나가지는데 해당 다이얼로그 아래에 취소 버튼이 있으면 좋을 것 같아요 !! ㅎㅎ
(CheaterFormDialog#createCancelButton()를 차용하면 좋을 것 같습니다)

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
init {
width = "800px"
height = "90%"
add(H2("지원자 정보 조회"), HorizontalLayout(createAddRecipients())).apply {
setWidthFull()
}
open()
}
init {
add(createHeader(), HorizontalLayout(createAddRecipients()), createButtons())
width = "800px"
height = "90%"
open()
}

위 코멘트에 이어서 다른 다이얼로그처럼 너비 높이 선언을 아래로 내려주어 사이즈를 맞추고
제안한 코드 같은 다른 다이얼로그와 유사한 구조로 변경해보는 것은 어떨까 싶어요.
정말 개인적인 생각이지만, 전반적인 뷰의 코드나 화면 통일성도 꽤 중요한 것 같아서,
CheaterFormDialog를 참고해보아도 좋을 것 같아요 !
리팩토링하는데 혹시나 도움이 될까 싶어 남기는 개인적인 의견이라 반영하지 않으셔도 좋습니다 !!
참고만 부탁드려요 :)
image
위와 같은 구조를 적용했을 때 모습입니다.

Copy link
Contributor

Choose a reason for hiding this comment

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

앗 그리고 기존에 검색 요소가 그리드보다 튀어나온 부분도 정렬해주면 좋을 것 같아요 !

Copy link
Contributor

Choose a reason for hiding this comment

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

와 마찌... 디테일한 리뷰 🔥
저도 전반적으로 동의합니다 👍
특히 개별 불러오기 창에서 취소하는 버튼은 필요해보여요. 다이얼로그의 크기도 어느정도 통일되면 더 좋을것 같구요!

Copy link
Contributor

@knae11 knae11 Sep 25, 2021

Choose a reason for hiding this comment

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

우어ㅣ..바딘은 마스터했군욬ㅋㅋㅋㅋㅋㅋㅋㅋㅋ 👍

그리고 선택후 닫기가 애매해서 마찌가 취소 버튼을 넣어준 것처럼 닫기 버튼 등이 있어서 모달이 닫히면 좋을 듯 싶어요!ㅎㅎㅎ

Copy link
Contributor Author

@Rok93 Rok93 Sep 25, 2021

Choose a reason for hiding this comment

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

그리드가 튀어나오는 부분 수정하고 취소버튼 추가했습니다 😃

Comment on lines 43 to 49
createPrimaryButton("추가") {
reloadComponent(mailTargets)
close()
},
createErrorButton("취소") {
close()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

이것도 개인적인 생각인데,
만약 다이얼로그 사이즈가 좀 준다면 셀렉트 박스랑 버튼이 같이있어 상단이 꽉차니
버튼 HorizontalLayout을 하단으로 내리는 것은 어떨까라는 생각이 드는데 요건 어떨까요 ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

가로 폭을 줄이다보니 자연스럽게 버튼들을 하단으로 내리는게 좋을 것 같네요 🤔


private fun createMailTargetsGrid(mailTargets: List<MailTargetResponse>): Component {
return Grid<MailTargetResponse>(10).apply {
addSortableColumn("이메일", MailTargetResponse::email)
Copy link
Contributor

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.

그렇네요 여기에서도 이름을 추가하면 좋을 것 같아요 😃

Comment on lines 62 to 65
private fun createEvaluationItem(): Select<Evaluation> {
return createItemSelect("평가")
}

Copy link
Contributor

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.

이 부분은 createItemSelect()를 필드(=evaluation)에 바로 할당하면 타입 추론을 할 수 없는 문제가 있어서 지금처럼 메서드로 뽑았는데... 혹시 바로 호출할 수 있는 방법이 있을까요 ? 🤔

image

Copy link
Contributor

Choose a reason for hiding this comment

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

@Rok93

    private val evaluation = createItemSelect<Evaluation>("평가")

이렇게 타입 명시를 해주면 될거에요 ! ㅎㅎ

private val recruitment = createRecruitmentItem(evaluation)
private val evaluationStatus = createEvaluationStatusItem(evaluation)
private val mailTargets: MutableList<MailTargetResponse> = mutableListOf()
private val currentMailTargets: VerticalLayout = VerticalLayout(createContent())
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private val currentMailTargets: VerticalLayout = VerticalLayout(createContent())
private val currentMailTargetsGrid: Grid<MailTargetResponse> = createMailTargetsGrid()

여기를 그리드로 바꾸고,

Comment on lines 90 to 93
currentMailTargets.apply {
this.removeAll()
this.add(createContent(mailTargetResponses))
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
currentMailTargets.apply {
this.removeAll()
this.add(createContent(mailTargetResponses))
}
currentMailTargetsGrid.apply {
setItems(mailTargetResponses)
}

setItem을 사용해 createContent() 메서드를 제거하는 방법도 있을 것 같은데 이런 방법은 어떨까요 ??

Copy link
Contributor Author

Choose a reason for hiding this comment

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

기존에는 VerticalLayout에 Component(아마 Span 이었던걸로..)를 각각 추가하다 보니 지금처럼 removeAll, add 메서드로 되어있었는데, VerticalLayout으로 감싸지 않고 바로 Grid를 사용한다면 마찌가 제안한대로 변경할 수 있을 것 같아요!


private fun createRemoveButtonRender(): Renderer<MailTargetResponse> {
return ComponentRenderer<Component, MailTargetResponse> { response ->
createPrimarySmallButton("삭제") {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
createPrimarySmallButton("삭제") {
createErrorButton("삭제") {

여기는 ErrorButton을 쓰면 좋을 것 같네요 ㅎㅎ

Copy link
Contributor Author

@Rok93 Rok93 Sep 25, 2021

Choose a reason for hiding this comment

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

처음엔 엄청 신경 썻던 부분인데... 어느 순간 별 생각없이 버튼을 만들고 있었네요 🙄
변경했습니다!

Copy link
Contributor

@Sehwan-Jang Sehwan-Jang left a comment

Choose a reason for hiding this comment

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

로키 어제 저녁부터 정말 고생하셨습니다 🔥
고생하신 만큼 코드도 많이 발전된것 같아 기분이 좋네요 ㅎㅎ

하지만 아직 몇군데 불필요하거나 추가되었으면 좋겠는 부분 코멘트 남겼어요!

}

private fun createMailForm(): Component {
val subjectText = VerticalLayout(H3("메일 제목"), subject).apply { setWidthFull() }
Copy link
Contributor

Choose a reason for hiding this comment

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

subject에 이미 라벨이 붙어있으니 H3 컴포넌트는 없어도 괜찮을 것 같아요~

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아직 헤더의 잔재가... 👀 H3 컴포넌트 삭제했습니다 👍

Comment on lines 79 to 92
return VerticalLayout(
subjectText,
sender,
recipientFilter,
VerticalLayout(mailTargetGrid),
mailBody,
VerticalLayout(uploadFile),
sendButton
).apply {
setSizeFull()
justifyContentMode = FlexComponent.JustifyContentMode.CENTER
defaultHorizontalComponentAlignment = FlexComponent.Alignment.CENTER
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

저도 마찌의 의견에 공감해요!
굳이 모든 요소들을 컴포넌트로 한번더 감쌀것 없이 바로 VerticalLayout을 반환하도록 한다면 훨씬 깔끔한 코드가 될 것 같네요 👍

Comment on lines 112 to 117
createSearchBar(labelText = "받는사람") {
if (it.isNotBlank()) {
mailTargets.add(MailTargetResponse("이름 없음", it))
mailTargetGrid.setItems(mailTargets)
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

이제 불러오기가 있으니 검색창은 없어도 될 것 같아요 :)

Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분은 없는 메일을 직접 입력하기 위한 창인 것 같아요!ㅎㅎ

Copy link
Contributor

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.

아... 이 검색창은 직접 입력을 위한 용도라서 그대로 둬야할 것 같아요!

}

private fun createSearchTargetComponent(): Button {
return createNormalButton("불러오기") {
Copy link
Contributor

Choose a reason for hiding this comment

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

사소한 부분이긴 하지만 불러오기 대신 개별 (메일) 검색 은 어떤가요?
불러오기라는 말을 보면 눌렀을때 자동으로 가져와질 것 같은 느낌이 들어서요~

Copy link
Contributor

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.

이전에 개별 조회, 그룹 조회 였던 걸로 기억나는데 (워낙 수정을 많이해서 이제 뭐가 뭐였는지 기억이 잘... 🙄) 제이슨이 불러오기는 어떻냐고 제안하셔서 변경하게 된거라서요.... 🤔

이 부분은 다른 크루들의 의견도 궁금하네요 👀

Comment on lines 23 to 30
init {
width = "800px"
height = "90%"
add(H2("지원자 정보 조회"), HorizontalLayout(createAddRecipients())).apply {
setWidthFull()
}
open()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

와 마찌... 디테일한 리뷰 🔥
저도 전반적으로 동의합니다 👍
특히 개별 불러오기 창에서 취소하는 버튼은 필요해보여요. 다이얼로그의 크기도 어느정도 통일되면 더 좋을것 같구요!

private val mailTargetService: MailTargetService,
private val mailProperties: MailProperties
) : BindingFormLayout<MailSendData>(MailSendData::class) {
private val subject: TextField = TextField("메일 제목")
Copy link
Contributor

@knae11 knae11 Sep 25, 2021

Choose a reason for hiding this comment

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

image
메일 제목 칸은 길게 하면 좋을 것 같은데 어떠세요..?ㅎㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 부분은 저도 많이 느꼈던 부분이라 현재 setWithFull() 조건을 줬습니다 : )

Copy link
Contributor

@knae11 knae11 left a comment

Choose a reason for hiding this comment

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

로키 👍 진짜 페이지 구성하느라 고생이 많네요..!!! 화이팅입니다!!
화면이 깔끔해졌어요!ㅎㅎㅎ 바딘 너무 어렵덴데 로키가 만들어준 내용들 덕분에 편하게 사용될 수 있을 것 같아요!! 진짜 큰 페이지 해주고 계시네요!!

저 바딘..잘 모르지만ㅋㅋㅋㅋㅋ 사용자 측면에서 조금 수정되었으면 좋겠을 의견 몇개 남겨보았어요~ 😄

Copy link
Contributor

@NewWisdom NewWisdom left a comment

Choose a reason for hiding this comment

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

로끼 !!!!
와우 진짜 수고 많았습니다 역시 로키 👏👏
화면이 많이 깔끔해졌네요 정말 많은 노고가 보입니다 🥲
코드상으로 확인해보면 좋을 것 같은 부분들을 조금 남겨보았는데,
확인하시고 동의하신다면, 반영해도 좋을 것 같아요 ㅎㅎ
수고 많으셨습니다 짱짱짱짱 🔥🔥

private val mailTargetService: MailTargetService,
private val reloadComponent: (List<MailTargetResponse>) -> Unit
) : Dialog() {
private val evaluation = createEvaluationItem()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private val evaluation = createEvaluationItem()
private val evaluation = createItemSelect<Evaluation>("평가")

메서드를 제거하고 이렇게 바꾸는건 어떨까요 ? ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

아... 메서드에 제네릭 타입을 저렇게 명시하면 바로 사용할 수 있는거였군요??
메서드에 바로 제네릭 타입을 지정할 수 있는지 전혀 몰랐네요 😧

코멘트 반영했습니다 😃


init {
add(createTitle(), createMailForm())
setWidthFull()
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
setWidthFull()

해당 설정은 제거되도 괜찮지 않을까요 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

제거했습니다 💪

Comment on lines 48 to 52
fun createUploadButton(
text: String,
receiver: MultiFileMemoryBuffer,
imageUploadSucceededListener: ImageUploadSucceededListener
): Upload {
Copy link
Contributor

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.

이 부분을 변경할 생각을 못했네요 🥲 모든 파일 형식을 다 업로드할 수 있어서 ImageUploadSucceededListener가 아닌 UploadSucceededListener로 변경하였습니다.

Comment on lines 31 to 33
).apply {
setWidthFull()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
).apply {
setWidthFull()
}
)

여기도 해당 설정은 적용되지 않는 것 같아요 ㅎㅎ!

Comment on lines 39 to 41
).apply {
setWidthFull()
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
).apply {
setWidthFull()
}
)

호잇

Copy link
Contributor Author

Choose a reason for hiding this comment

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

호잇!

Comment on lines 48 to 52
private fun createEditAndDeleteButton(): Renderer<ApplicantResponse> {
return ComponentRenderer<Component, ApplicantResponse> { applicantResponse ->
HorizontalLayout(createTargetAddButton(applicantResponse))
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

네이밍을 보면 수정, 삭제 버튼을 만들 것 같은데 추가 버튼을 생성하고 있어요 ~

Copy link
Contributor Author

@Rok93 Rok93 Sep 30, 2021

Choose a reason for hiding this comment

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

에고... 원래 추가 or 삭제 버튼을 생성하게하는 기능인데... 총체적 난국이네요 add가 아닌 edit에 심지어 or가 아닌 and ... ㅋㅋㅋㅋㅋㅋㅋㅋ 대체 무슨 생각을 하면서 만든건지...

바딘의 위험성 ⚠️

}
}

private fun createRecipientFilter(): Component {
Copy link
Contributor

Choose a reason for hiding this comment

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

야악간 개인적인 의견인데 createTargetFilter 등 처럼 바꾸는 것은 어떨까요 ??
여기서 메일 대상자를 target으로 대부분 지칭하고 있는데
Recipient과 Target이 혼용되고 있으니 헷갈릴 수도 있을 것 같아요 ㅎㅎ

Copy link
Contributor Author

Choose a reason for hiding this comment

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

처음에 Recipient로 명칭을 픽스하려고했는데, 여기저기 Target이라는 표현들이 많다보니 저도모르게 혼용하면서 썻던 것 같네요.
전체적으로 그냥 target 혹은 Recipient가 아닌 mailTarget이라는 용어로 변경했습니다.

Comment on lines 110 to 111
createSearchTargetComponent(),
createGroupMailTargetComponent()
Copy link
Contributor

Choose a reason for hiding this comment

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

둘이 비슷한 (개별) 불러오기, 그룹 불러오기 버튼인데 약간
createIndividual~Button(), createGroup~Button()
이런식으로 이름에 어떤 요소인지 드러내고 통일성을 가져가는 건 어떠신가요 ??

@woowahan-pjs
Copy link
Contributor

woowahan-pjs commented Oct 4, 2021

  • BeanValidationBinder가 사용하는 RequiredFieldConfigurator@NotNull, @NotEmpty, @Size이며, MailData의 프로퍼티는 널이 될 수 없는 타입이므로 @NotNull에서 @NotEmpty로 변경
    • @NotBlank도 지원하도록 커스텀하면 어떨까?
  • MailData의 프로퍼티 이름과 매칭되는 MailFormView의 프로퍼티 이름을 통일
  • 컬렉션과 짝을 이루는 Grid가 함께 작동할 수 있도록 확장 함수 분리 및 범위 지정 함수 사용
  • GridDataProvider 사용 (https://vaadin.com/components/vaadin-grid/java-examples/assigning-data)
  • 다른 View의 Grid 내부와 마찬가지로 Grid 내부의 버튼을 작은 버튼으로 변경
    • BindingFormLayout을 상속한 'Form'과 페이지인 'View'를 분리하면 어떨까?
  • 이미 기본 또는 불필요한 CSS 옵션 제거
  • 같은 위치에 있거나, 동일한 CSS 옵션을 주기 위함이 아닌 불필요한 HorizontalLayout 삭제
  • (여러 함수에서 접근하지 않거나, 바인딩 목적으로 사용하지 않아) 프로퍼티로 관리할 필요가 없는 Vaadin의 컴포넌트 타입의 프로퍼티 삭제
  • GroupMailTargetDialogIndividualMailTargetDialog의 함수 이름과 함수 위치 통일
    • 추상 클래스로 빼면 어떨까?
  • 상수 및 최상위 함수로 뺄 수 있는 부분 분리
  • 애너테이션이 붙은 프로퍼티 사이에 한 줄 바꿈 적용
  • 후행 쉼표 삭제

Copy link
Contributor

@woowahan-pjs woowahan-pjs left a comment

Choose a reason for hiding this comment

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

고생하셨습니다. 👍

@woowahan-pjs woowahan-pjs changed the title feat: implement send email page feat(mail): implement the send mail view Oct 4, 2021
@woowahan-pjs woowahan-pjs merged commit 1c075b7 into develop Oct 4, 2021
@woowahan-pjs woowahan-pjs deleted the feature/send-mail-page branch October 4, 2021 15:36
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.

5 participants