Skip to content

Conversation

@yangwooseong
Copy link
Contributor

@yangwooseong yangwooseong commented Sep 5, 2023

Self Checklist

  • I wrote a PR title in English.
  • I added an appropriate label to the PR.
  • I wrote a commit message in English.
  • I wrote a commit message according to the Conventional Commits specification.
  • I added the appropriate changeset for the changes.
  • [Component] I wrote a unit test about the implementation.
  • [Component] I wrote a storybook document about the implementation.
  • [Component] I tested the implementation in various browsers.
    • Windows: Chrome, Edge, (Optional) Firefox
    • macOS: Chrome, Edge, Safari, (Optional) Firefox
  • [New Component] I added my username to the correct directory in the CODEOWNERS file.

Related Issue

Summary

  • bezier-figma-pluain의 실행 속도와 구조를 개선합니다.
  • 아이콘 마다 git blob 을 만드는 과정을 없애고 깃헙 액션에서 아이콘을 생성하도록 합니다.

Demo

개인 레포지토리 타겟으로 동작 시연 영상

2023-09-05.4.55.03.mov

깃헙 액션 다 돌고난 후

Details

  • 기존의 플러그인 동작방식은, 플러그인 피그마 API 를 통해 선택한 아이콘들을 모두 svg 로 변환하여 이걸 git blob 으로 변환한 후 github REST API 를 통해 pr 을 만드는 과정을 거쳤습니다. 그런데 500개가 넘는 아이콘에 대하여 /git/blobs API를 모두 호출해야 해서 시간이 많이 소요되었습니다. (대략 9분정도 소요)

  • 이번 pr 로 동작방식이 다음과 같이 변경되었습니다.

    1. Node.exportAsync 메서드를 통해 선택한 아이콘들을 svg string으로 변환
    2. 아이콘 이름과 svg string을 키 밸류로 하는 json 객체(icons.json)생성
    3. github API 를 통해 icons.json만 변경사항으로 가지는 pr 생성
    4. pr 이 생성되면 github action 이 트리거 되어 icons.json 파일로부터 각 아이콘 이름을 파일 이름으로, svg string 을 파일 내용으로 하여 아이콘 파일을 생성하는 스크립트(generate-icon-files.js) 실행
    5. icons.json 삭제하는 커밋 생성
  • 이 과정이 다 도는데 10초 정도가 걸리게 됩니다. 또한, icons.json을 보고 추가로 github action 을 트리거할 수 있기 때문에, changeset 생성이나 적절한 description 생성같은 기능도 추후에 덧붙이기 용이한 구조로 변경되었습니다.

  • 기존의 플러그인이 생성한 icon 파일과 변경사항이 없기를 원했지만, devices.svg 파일 한개에 대해서는 diff 가 발생하는 것을 막지 못했습니다(#). 아마 figmaAPI를 호출해서 svg 로 변환하는 것과 이번에 사용한 Node.exportAsync 의 결과물이 완전히 일치하지 않아서 그런 것 같습니다. 겉으로 보이는 것은 완전히 동일해서 diff 가 생겨도 무방하지 않을까 생각합니다만, 새로운 플러그인이 배포되면 한번 조정하는 작업이 필요해 보입니다.

Breaking change or not (Yes/No)

No

References

@yangwooseong yangwooseong added enhancement Issues or PR related to making existing features better bezier-figma-plugin Issue or PR related to bezier-figma-plugin labels Sep 5, 2023
@yangwooseong yangwooseong self-assigned this Sep 5, 2023
@changeset-bot
Copy link

changeset-bot bot commented Sep 5, 2023

🦋 Changeset detected

Latest commit: 4d45640

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
bezier-figma-plugin Minor
@channel.io/bezier-icons Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@codecov
Copy link

codecov bot commented Sep 5, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (772d763) 87.00% compared to head (4d45640) 87.00%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1596   +/-   ##
=======================================
  Coverage   87.00%   87.00%           
=======================================
  Files         281      281           
  Lines        3879     3879           
  Branches      817      817           
=======================================
  Hits         3375     3375           
  Misses        430      430           
  Partials       74       74           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

githubToken: string
progress: ReturnType<typeof useProgress>['progress']
}) {
const githubAPI = useGithubAPI({
Copy link
Contributor Author

Choose a reason for hiding this comment

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

useGithubAPI가 굳이 hook일 필요는 없을 것 같은데 혹시 이유가 있었는지 궁금합니다 @sungik-choi

Copy link
Contributor

Choose a reason for hiding this comment

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

그러게요..? 굳이 훅이여야될 이유는 없는 거 같아요. 그 당시 함수형 컴포넌트 안에서 써야지 -> 훅으로 만들자 라는 사고회로를 통해 훅으로 만들었던게 아닐까 싶어요 😅

@yangwooseong yangwooseong marked this pull request as ready for review September 5, 2023 11:14
restore-keys: |
${{ runner.os }}-yarn-
- name: Install Dependencies
Copy link
Contributor Author

Choose a reason for hiding this comment

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

이 과정에서 거의 대부분의 시간을 할애하고 있었어서 yarn script 대신 node 로 직접 실행하는 걸로 변경합니다.

Copy link
Contributor

@sungik-choi sungik-choi left a comment

Choose a reason for hiding this comment

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

👍 너무 멋집니다~!
마무리되면 피그마에 배포하고, Bezier 방에도 릴리즈 내용 공유해보시죠!

githubToken: string
progress: ReturnType<typeof useProgress>['progress']
}) {
const githubAPI = useGithubAPI({
Copy link
Contributor

Choose a reason for hiding this comment

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

그러게요..? 굳이 훅이여야될 이유는 없는 거 같아요. 그 당시 함수형 컴포넌트 안에서 써야지 -> 훅으로 만들자 라는 사고회로를 통해 훅으로 만들었던게 아닐까 싶어요 😅

on:
push:
branches:
- icon-update-*
Copy link
Contributor

@sungik-choi sungik-choi Sep 8, 2023

Choose a reason for hiding this comment

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

아하.. 브랜치 이름 + 변경 파일로 필터를 거셨군요 👍 생각 못했네요.

  • icon-update 브랜치명이 피그마 플러그인의 newBranchName 과 같아야한다는 주석이 있어도 좋을 거 같습니다.

Comment on lines 28 to 38
- name: Generate Svg files from icons.json
run: |
node packages/bezier-icons/scripts/generate-icon-files.js
git add .
git commit -m "feat(bezier-icons): generate icon files from icons.json"
- name: Delete icons.json files
run: |
git rm packages/bezier-icons/icons.json
git commit -m 'feat(bezier-icons): remove icons.json'
git push
Copy link
Contributor

Choose a reason for hiding this comment

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

👍👍👍

매우 마이너: 커밋 메세지의 따옴표를 ' -> " 로 통일하면 좋겠습니다 ㅎㅎ


const generateSVGFilesFromMap = () => {
Object.entries(svgByName)
.sort(([iconName1], [iconName2]) => iconName1.localeCompare(iconName2))
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.

안해도 됩니다! 착각한 것 같습니다

const generateSVGFilesFromMap = () => {
Object.entries(svgByName)
.sort(([iconName1], [iconName2]) => iconName1.localeCompare(iconName2))
.map(makeSvgFiles)
Copy link
Contributor

Choose a reason for hiding this comment

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

forEach ?

const createPRWithSvgMap = useCallback(async (svgByName: SvgByName) => {
const mainBranch = await progress({
callback: getMainBranch('main'),
title: '📦 깃헙에서 아이콘 정보를 읽는 중...',
Copy link
Contributor

Choose a reason for hiding this comment

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

마이너: '깃헙에서 정보를 가져오는 중', '깃헙에 접속 중' 같은 문구가 함수가 하는 일에 더 적절해보였어요. 아이콘 정보를 읽지는 않아서..!

@sungik-choi
Copy link
Contributor

sungik-choi commented Sep 8, 2023

기존의 플러그인이 생성한 icon 파일과 변경사항이 없기를 원했지만, devices.svg 파일 한개에 대해서는 diff 가 발생하는 것을 막지 못했습니다(#). 아마 figmaAPI를 호출해서 svg 로 변환하는 것과 이번에 사용한 Node.exportAsync 의 결과물이 완전히 일치하지 않아서 그런 것 같습니다. 겉으로 보이는 것은 완전히 동일해서 diff 가 생겨도 무방하지 않을까 생각합니다만, 새로운 플러그인이 배포되면 한번 조정하는 작업이 필요해 보입니다.

이건 확인해봤는데 아이콘이 실제로 디자인 변경이 있어서 그런 거 같아요. 우측변이 우측으로 약~~간 이동했네요.

@yangwooseong yangwooseong force-pushed the feat/enhance-figma-plugin-workflow branch 3 times, most recently from 2fd34d2 to 0e3ce8f Compare September 11, 2023 06:47
- 깃헙에서 아이콘 정보를 읽는 중... ->  깃헙에서 정보를 가져오는 중...
@yangwooseong yangwooseong force-pushed the feat/enhance-figma-plugin-workflow branch from 0e3ce8f to 4d45640 Compare September 11, 2023 06:52
@yangwooseong yangwooseong merged commit 9d44070 into channel-io:main Sep 11, 2023
sungik-choi pushed a commit that referenced this pull request Sep 12, 2023
This PR was opened by the [Changesets
release](https://github.com/changesets/action) GitHub action. When
you're ready to do a release, you can merge this and the packages will
be published to npm automatically. If you're not ready to do a release
yet, that's fine, whenever you add more changesets to main, this PR will
be updated.


# Releases
## @channel.io/[email protected]

### Minor Changes

- Add Banner to iconName transformation target
([#1562](#1562)) by
@yangwooseong

## @channel.io/[email protected]

### Minor Changes

- Add script for generating svg file from icons.json
([#1596](#1596)) by
@yangwooseong

## @channel.io/[email protected]

### Minor Changes

- Allow `iconName` prop of `Button`, `Banner`, `SectionLabel` component
to include `BezierIcon` type
([#1562](#1562)) by
@yangwooseong

### Patch Changes

- Fixed a rendering bug that occurs when using `react-resize-detector`
and `asChild`prop of `radix-ui` simultaneously.
([#1577](#1577)) by
@yangwooseong

- Removed indicator adjusting logic by `react-resize-detector` using css
transform property

- Replace `@channel.io/react-docgen-typescript-plugin` with Storybook's
`reactDocgen` option
([#1594](#1594)) by
@Dogdriip

## [email protected]

### Minor Changes

- Enhance bezier-figma-plugin running performance
([#1596](#1596)) by
@yangwooseong
- Remove svg extract process using FigmaAPI and merely send json file
that contains svg string
    -   Make icon files based on given json file during Github Action

### Patch Changes

-   Updated dependencies
    -   @channel.io/[email protected]
    -   @channel.io/[email protected]

Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@yangwooseong yangwooseong deleted the feat/enhance-figma-plugin-workflow branch September 15, 2023 05:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bezier-figma-plugin Issue or PR related to bezier-figma-plugin enhancement Issues or PR related to making existing features better

Projects

No open projects
Archived in project

Development

Successfully merging this pull request may close these issues.

2 participants