feat: improve the side navigation ui #650
Conversation
| } | ||
| } | ||
|
|
||
| private fun createMenu(list: List<MenuItem>): Tabs { |
There was a problem hiding this comment.
a: 호출하는 메서드와 가깝게 두면 읽기가 더 편할 것 같아요!
(다른 메서드들도 마찬가지로요!)
| private val topMenu = createMenu(createTopMenuItems()) | ||
| private val bottomMenu = createMenu(createBottomMenuItems()) |
There was a problem hiding this comment.
a: topMenu, bottomMenu 보다 더 직관적인 변수명으로 짓는 것이 어떨까요?
ex) topMenu : 선발진행관련 변수명
There was a problem hiding this comment.
지금은 선발진행관련 메뉴 아이템들이 해당 공간에 존재하지만 추후 어떤 메뉴 아이템이 들어올지 변경될 수 있고 그렇다면 네이밍 또한 변할 것 같아 이렇게 포괄적인 네이밍을 지었던 것 같습니다.
| import com.vaadin.flow.component.tabs.Tabs | ||
| import support.views.createTabs | ||
|
|
||
| private const val UNSELECT_ALL: Int = -1 |
There was a problem hiding this comment.
UNSELECT_ALL 이라는 네이밍과 -1 이라는 값이 잘 이해가 가지 않아요.
사용하는 곳에서는 현재 선택된 INDEX를 -1 로 바꾸로 바꾸는 용도로 사용하는 것 같아서요!
a: 만약 다른 메뉴에서 현재 선택된 INDEX 를 -1로 바꾸는 것이면 UNSELECTED 정도가 괜찮지 않을까요?
(제가 잘못 이해한 것일수도 있습니다. 😅)
There was a problem hiding this comment.
바딘에서 제공하는 Tabs에 나와있는 setSelectedIndex의 javaDocs에는 -1 to unselect all 이라고 적혀있어서 이를 참고해서 네이밍했습니다 😋
|
PR 제목 수정해주세요. |
kbsat
changed the title
kbsat
force-pushed
the
feature/side-bar
branch
from
e3d9f24 to
fbe18e8
Compare
Resolves #617
해결하려는 문제가 무엇인가요?
어떻게 해결했나요?
Anchor대신RouterLink를 사용하여 화면이 새로고침되는 것을 막아보았습니다.RouterLink설정을 위해 VerticalLayout과 HasUrlParameter 를 하나로 묶는 추상클래스인HasUrlParamLayout을 생성했습니다.🚨🚨주의 : 아직 관리자 탭은 머지가 되지 않았기 때문에 클릭 시 메일 관리 페이지가 보입니다. 이후 관리자 기능 머지 후에 수정이 필요합니다(10.26) 새로 리베이스 하면서 AdministratorsView로 이동되는 것도 반영했습니다.
어떤 부분에 집중하여 리뷰해야 할까요?
RCA 룰
r: 꼭 반영해 주세요. 적극적으로 고려해 주세요. (Request changes)
c: 웬만하면 반영해 주세요. (Comment)
a: 반영해도 좋고 넘어가도 좋습니다. 그냥 사소한 의견입니다. (Approve)