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

fix: floatingRow shows incompletely for multiple rows(fix #1427) #1477

Merged
merged 1 commit into from
Oct 7, 2021

Conversation

jajugoguma
Copy link
Contributor

Please check if the PR fulfills these requirements

  • It's submitted to right branch according to our branching model
  • It's right issue type on title
  • When resolving a specific issue, it's referenced in the PR's title (e.g. fix #xxx[,#xxx], where "xxx" is the issue number)
  • The commit message follows our guidelines
  • Tests for the changes have been added (for bug fixes/features)
  • Docs have been added/updated (for bug fixes/features)
  • It does not introduce a breaking change or has description for the breaking change

Description

  • Fix a problem in which floatingRow is incompletely displayed for multiple rows.

As-Is
as-is

To-Be
to-be


Thank you for your contribution to TOAST UI product. 🎉 😘 ✨

Copy link

@adhrinae adhrinae left a comment

Choose a reason for hiding this comment

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

리뷰 완료합니다.

그런데 Floating row가 별도의 스타일을 가진 것 처럼 보이는데, 원래 플로팅 전의 열 스타일을 그대로 재사용하거나 할 수는 없는건가요? 재사용하고 opacity만 약간 옅게 하는 것도 괜찮을 것 같은데요.

Copy link

@dongsik-yoo dongsik-yoo 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 +973 to +976
overflow: hidden;
line-height: normal;
vertical-align: middle;
text-align: center;
Copy link
Contributor

Choose a reason for hiding this comment

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

vertical-align말고 나머지 스타일은 없어도 될 것 같은데 추가된 이유가 있나요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

overflow의 경우 요소를 초과하는 정보를 감추기 위해 사용했습니다.
text-align은 로우 헤더에 해당하는 요소를 중앙 정렬하기 위해 사용했습니다. 다른 요소의 경우 오버랩을 통해 좌측 정렬하도록 했습니다.
line-height의 경우 개행된 데이터가 표시된 경우 상위 요소에 설정한 값이 상속되어 정상적으로 표시 되지 않기 때문에 이를 방지하기 위한 용도로 추가했습니다.

Copy link
Contributor

@js87zz js87zz Oct 7, 2021

Choose a reason for hiding this comment

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

아 넵 이해했습니다. 정렬은 left를 기본으로 하고 로우 헤더만 중앙 정렬하는게 어떤가요? 일반적으로 기본 셀이 더 많을 거라서요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

일반 셀은 .tui-grid-floating-cell .tui-grid-cell-content로 쉽게 접근할 수 있지만 로우 헤더는 그렇지 않아서 위와 같이 작성 했습니다.
floating cell 생성 시 로우 헤더인지 판별 후 인라인 스타일로 지정하는 방식으로 변경하는 것이 더 나을까요?

Comment on lines +980 to +982
padding: 0 5px;
word-break: break-all;
text-align: start;
Copy link
Contributor

Choose a reason for hiding this comment

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

패딩은 간격 조정때문에 넣은 것 같은데 나머지 스타일은 어떤 용도일까요~? 기본적으로 좌측 정렬(ltl 기준)이라 start는 제거해도 되겠네요

Copy link
Contributor Author

Choose a reason for hiding this comment

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

floating layer가 기존 row와 동일하게 보여주기 위해 추가된 스타일 입니다.
좌측 정렬의 경우 row header에 해당하는 요소는 중앙 정렬로 하고, 그렇지 않은 경우 스타일 오버랩으로 좌측정렬을 시켜주었습니다.

Copy link
Contributor

@js87zz js87zz left a comment

Choose a reason for hiding this comment

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

리뷰 완료입니다! 일반 로우, 트리 다 잘 동작하네요ㅎㅎ 불필요한 스타일이 있는 것 같아 해당 부분만 확인 부탁드려요~

@jajugoguma jajugoguma merged commit f2d8df6 into master Oct 7, 2021
@jajugoguma jajugoguma deleted the fix/floatingRow-incompletely-shows branch October 7, 2021 09:45
@jajugoguma jajugoguma restored the fix/floatingRow-incompletely-shows branch October 7, 2021 10:33
@jajugoguma jajugoguma deleted the fix/floatingRow-incompletely-shows branch October 28, 2021 01:30
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.

4 participants