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

[view] 상단 branch명 길이 관련 suffix 추가 #721

Merged
merged 13 commits into from
Sep 13, 2024
Merged

Conversation

xxxjinn
Copy link
Contributor

@xxxjinn xxxjinn commented Sep 10, 2024

Related issue

close #707

Result

image

Work list

  • BranchSelector.const.ts 파일을 만들어 slice constant를 생성해두고, 일단 15로 임의로 정해두었습니다.
  • branch명이 15글자 이상일 때에는 suffix로 (...)를 붙였습니다.
  • tooltip은 디자인 시스템 팀원 분들의 의견에 따라 mui tooltip을 따로 사용하지 않고, 태그에 title을 넣는 방식을 사용했습니다.

Discussion

@xxxjinn xxxjinn added this to the v0.7.2 milestone Sep 10, 2024
@xxxjinn xxxjinn self-assigned this Sep 10, 2024
@xxxjinn xxxjinn requested review from a team as code owners September 10, 2024 13:08
pcwadarong
pcwadarong previously approved these changes Sep 10, 2024
Copy link
Member

@pcwadarong pcwadarong left a comment

Choose a reason for hiding this comment

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

오 좋아요~~ LGTM~~~

>
{option}
{option.length <= SLICE_LENGTH ? option : `${option.slice(0, SLICE_LENGTH)}...`}
Copy link
Contributor

Choose a reason for hiding this comment

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

혹시 text-overflow: ellipsis;는 사용 불가할까요? 👀
https://developer.mozilla.org/en-US/docs/Web/CSS/text-overflow

Copy link
Contributor Author

@xxxjinn xxxjinn Sep 11, 2024

Choose a reason for hiding this comment

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

@choisohyun

MenuProps={{
    PaperProps: {
      sx: {
        backgroundColor: "#212121",
        color: "white",
        marginTop: "1px",
        "& .MuiMenuItem-root": {
          backgroundColor: "#212121 !important ",
          "&:hover": {
            backgroundColor: "#333333 !important",
          },
        },
        "& .MuiMenuItem-root.Mui-selected": {
          backgroundColor: "#333333 !important",
        },
      },
    },
}}
  1. 현재 mui select를 사용하고 있고, 이에 따라 dropdown item의 스타일링을 위와 같이 select 태그에 props로 주고 있는 상황이었어서 말씀해주신 속성을 추가해 굳이 sx부분의 코드를 더 복잡하게 보이게 하고 싶지 않았습니다. 아시겠지만 text-overflow만 추가되는 게 아니라 width, overflow 등의 속성도 부가적으로 추가가 되어야 하니까요!! (해당 pr과는 관련 없지만 그래서 select 모듈화도 진행해 볼 계획입니다!)

  2. dropdown box의 width를 어떻게 할 지(위의 select에 맞출지, 현재 pr에 올린대로 둘지)도 고민해보고 건드리지 않는 방향을 선택했고, 이에 따라 slice_length라는 constant를 통해 누구든 자르고 싶은 글자 수를 수정할 수 있도록 해두었습니다.


image

말씀하신대로 text-overflow를 적용하는 김에 width도 위의 select box의 width에 맞추어서 수정한 상태인데 요게 더 괜찮으시다면 text-overflow를 적용해 수정하겠습니다! 의견 말씀해주세요

Copy link
Contributor

Choose a reason for hiding this comment

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

아하 지금 상황이면 코드 복잡도가 더 늘어날 수 있겠군요
ellipsis를 사용하는건 width가 계속 바뀌는 반응형 형태에 대응하는 경우가 많았던 것 같아서 현재 상태에서는 이대로 가도 괜찮을 것 같습니다!
select 부분 width가 자주 바뀐다면 불편할 수 있을 것 같은데 저부분은 그럴 것 같진 않네용

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ellipsis를 사용하는건 width가 계속 바뀌는 반응형 형태에 대응하는 경우가 많았던 것 같아서

앗 새로운 지식 알아갑니다 감사합니다!!👍👍🙂

ytaek
ytaek previously approved these changes Sep 11, 2024
Copy link
Contributor

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

LGTM!!

@@ -1,4 +1,4 @@
{
"branchList": ["main", "develop", "release", "origin/HEAD"],
"branchList": ["main", "develop", "release", "origin/HEAD", "test branch name for long branch names"],
Copy link
Contributor

Choose a reason for hiding this comment

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

(궁금) git branch name에 공백이 들어갈 수 있을까요?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

헉!!! 예리하시네요... git branch name에는 공백이 들어갈 수 없습니다,, https://git.jiny.dev/branch/create/name.html
(등등 특정 기호도 못들어간다고 하네요..!! 덕분에 알게 되었습니다)

테스트 브랜치 명 수정했습니다!!! 감사합니다😖 92f8b2b

@xxxjinn xxxjinn dismissed stale reviews from ytaek and pcwadarong via 92f8b2b September 11, 2024 08:55
Copy link
Contributor

@choisohyun choisohyun 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

@ytaek ytaek left a comment

Choose a reason for hiding this comment

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

LGGTM!!!!

@xxxjinn xxxjinn merged commit 011bbf8 into githru:main Sep 13, 2024
2 checks passed
HIITMEMARIO pushed a commit to HIITMEMARIO/githru-vscode-ext that referenced this pull request Sep 27, 2024
* style: edit author bar chart width (200 to 250)

* style: edit select-box styling

* feat: change select box from select tag to mui select

* style: edit style branch select dropdown (add marginTop)

* feat: edit branch-list fake-assets for branch name length test

* feat: add BranchSelector.const file for branch name slice length

* feat: add suffix (...) to branch name

* refactor(view): edit branch-list fake-assets
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.

[view] 상단 branch명 관련
4 participants