오픈 소스 첫 기여(Enact)

hodu·2023년 10월 22일

Library

목록 보기

3/5

✅ 개요

Enact 프레임워크에 있는 라이브러리 spotlight에 기여했다.

과제에서 스크롤 이벤트와 함께 포커스 스크롤도 같이 작동하면서 충돌이 일어났다.
포커스 스크롤을 제어해야했다. spotlight의 해당 기능이 없었고, 직접 요소에 focus를 주어서 preventScroll로 제어를 했다.

커스텀하여 포커스 스크롤을 제어 하는 점이 아쉬웠다.
spotlight를 만족하면서 쓰고 있었고, 재사용 의사가 있었기에
preventScroll 기능을 추가하기로 하였다.

@enact/spotlight

5-방향 탐색 및 초점 제어를 위한 확장 가능한 라이브러리입니다.
키보드 또는 텔레비전 리모트 컨트롤을 사용하여 애플리케이션을 탐색할 수 있도록 하는 확장 가능한 유틸리티입니다. UP, DOWN, LEFT, RIGHT, RETURN 키로부터의 입력을 처리하며, Spotlight는 마우스가 있는 컴퓨터와 비슷한 탐색 경험을 제공합니다.

✅ 코드 리뷰

코드를 수정하기 전에 제일 우선시 한 것은 Side Effect와 기존 코드 재사용이었다.

그래서 먼저 기존 코드를 리뷰했다.

focus: function (elem, containerOption = {})

기존 함수에서는 1번째 parameter로 요소를 받는다.
2번째 parameter에서는 containerOption을 받는다.

containerOption은 2가지 옵션을 받는데

enterTo: 포커스가 이동할 때 어떤 요소에 포커스를 맞출지 결정합니다.
toOuterContainer: 적절한 대상이 없을 경우 외부 컨테이너까지 재귀적으로 검색합니다.

여기에 추가하는 방식으로 preventScroll을 넣으면 되겠다고 생각했다.
그래서 따로 parameter를 추가하지 않았다.

spotlight focus 함수 안에서 focusElement를 통해 focus를 주는데,
이 함수를 살펴보아야했다.

const focused = focusElement(target, nextContainerIds)

현재 2개를 인자를 받는데 하나는 타겟 요소, 두번째는 컨테이너였다.

function focusElement (elem, containerIds, fromPointer)

해당 함수로 들어가니 3개의 인자를 받았다.
3번째 인자는 마우스 클릭 여부였다.

그래서 4번째 인자로 preventScroll을 구상했다.

focusElement 로직을 보니 preventScroll를 주는 상황이 존재했다.

const focusOptions = 
      isWithinOverflowContainer(elem, containerIds) ? {preventScroll: true} : null;

이 isWithinOverflowContainer 함수는 주어진 대상이 오버플로우(overflow) 설정이 된 컨테이너 내부에 있는지 확인하는 역할이다.

밖에 있으면 preventScroll을 주고 내부에 있으면 null를 밷는다.

그래서 나는 이 로직에 preventScroll 여부를 받아서 OR 연산자로 수동적으로 추가할 수 있도록 구현하였다.

👉 개선 코드

//변경 전
function focusElement (elem, containerIds, fromPointer) {
const focusOptions = 
      isWithinOverflowContainer(elem, containerIds) ? {preventScroll: true} : null;

//변경 후
function focusElement (elem, containerIds, fromPointer, preventScroll = false) {

const shouldPreventScroll = 
      isWithinOverflowContainer(elem, containerIds) || preventScroll;
const focusOptions = shouldPreventScroll ? { preventScroll: true } : null;

preventScroll을 옵션으로 받고, 기본 값을 false로 잡았다.
(focusElement를 사용하는 다른 코드에 영향을 주지 않는 것을 명확하게 하기 위함이었다.)

매개변수로 PreventScroll을 받았을 때와 화면 밖으로 나갔을 때(isWithinOverflowContainer)를 고려할 수 있도록
논리 OR 연산자로 받아 주었다.

//변경 전
const focused = focusElement(target, nextContainerIds);

//변경 후
const focused = 
      focusElement(target, nextContainerIds, false, containerOption.preventScroll);

그리고 4번째 인자 값을 받기 위해서 3번째 인자값을 추가했다.
focusElement 함수를 사용하는 다른 값들은 4번째 인자를 사용하지 않기때문에 유지하였다.

💡 Contribution Guide

이 부분이 제일 힘들었다.
왜냐하면 기여가 처음이어서 꼼꼼히 여러번 체크하였다.

먼저
Enact는 서명을 해주고,
Documentation Style Guide와 Documenting Changes에 맞추어서 설명도 추가해주어야했다.

@param {Object} [containerOption] The object including enterTo, toOuterContainer, and preventScroll.
@param {Boolean} [containerOption.preventScroll] Prevents automatic scrolling when focusing the element.

preventScroll을 Containr Option에 있다고 알리는 주석과
PreventScroll에 대한 설명을 적었다.

## Changed
Added preventScroll parameter to `focusElement` for enhanced scroll control.
Extended `focus` in `spotlight/Spotlight` with a containerOption parameter that includes preventScroll.

CHANGELOG에도 해당 2개가 수정된 내용을 작성하였다.

🚀 Pull request

리뷰어 2분과 이야기를 나누며 내가 겪었던 문제점과 코드를 작성할 때 고려했던 점에 대해서 설명하였다.

이야기를 나눠보면서 내가 겪었던 시나리오에 대해서 설명했다.

부족했던 부분이 리뷰어를 위해 상황을 재현할 코드를 준비안한 것과 시나리오에 대한 세세한 설명을 적지 않았던 점이 아쉬었다.

Separate PRs for focusElement and spotlight.focus
Considering splitting this PR into two: one for focusElement and another for spotlight.focus. This would allow for more focused review and testing of each feature.

좋았던 점은 mdn 문서를 첨삭한 것과 내가 고려한 상황에 대한 설명이 잘적어서 마음에 들었다.