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.

Sign up for GitHub

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

Jump to bottom

[Playground] Component - Tab for system message and parameters (UI only) #225 #244

Merged
merged 11 commits into from
Aug 31, 2024
Merged

[Playground] Component - Tab for system message and parameters (UI only) #225 #244

merged 11 commits into from
Aug 31, 2024

Conversation

5jisoo
Copy link
Contributor

@5jisoo 5jisoo commented Aug 20, 2024 •
edited
Loading

Description

Summary of Changes

  • ConfigTabComponent 추가 :
    FluentUI의� FluentTab을 사용하였으며, 탭 전환을 확인할 수 있도록 OnTabChange 이벤트를 추가해두었습니다.

→ Home page에 컴포넌트 추가해서 확인하였습니다!

Questions

  • Tab Component 테스트 작성에서 감을 잡지 못하고 있습니다

클릭한 tab의 id에 따라

<p id ="active-tab">[TEST] Active tab changed to: @SelectedTab?.Id</p>

이 부분의 텍스트에 해당 tab의 id가 포함되는가? 에 대해 작성하고자 했는데,
너무 하드 코딩의 테스트가 되어버리진 않을까 걱정되고, 근본적으로 이렇게 작성하는 게 맞는지도 의문이 드는 상태입니다!

@justinyoo justinyoo linked an issue Aug 21, 2024 that may be closed by this pull request
[Playground] Component - Tab for system message and parameters (UI only) #225
Closed
justinyoo
justinyoo requested changes Aug 21, 2024
View reviewed changes
Copy link
Contributor

@justinyoo justinyoo left a comment

Choose a reason for hiding this comment

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

@5jisoo 코멘트 남겨뒀습니다. 일단 이것 전에 /playground 페이지 콤포넌트 만드는 PR 먼저 시작해 볼까요?

src/AzureOpenAIProxy.PlaygroundApp/Components/UI/ConfigTabComponent.razor Outdated Show resolved Hide resolved
src/AzureOpenAIProxy.PlaygroundApp/Program.cs Outdated Show resolved Hide resolved
src/AzureOpenAIProxy.PlaygroundApp/Components/Pages/Home.razor Outdated Show resolved Hide resolved
@justinyoo
Copy link
Contributor

#6 👉 #245 👉 #246 순서로 참고해 보시고, 아무래도 이 PR보다 #246 관련 PR을 먼저 만들고 거기서 작업해 보는 게 나을 듯 합니다. 이건 그동안 홀드해 둘까요?

@5jisoo 5jisoo mentioned this pull request Aug 21, 2024
Closed
@justinyoo
Copy link
Contributor

@5jisoo 컨플릭 해결해 주세요!

justinyoo
justinyoo requested changes Aug 30, 2024
View reviewed changes
Copy link
Contributor

@justinyoo justinyoo left a comment

Choose a reason for hiding this comment

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

중복 라인 제거하시고, 이제 테스트 추가해 주세요!

src/AzureOpenAIProxy.PlaygroundApp/Components/_Imports.razor Outdated Show resolved Hide resolved
src/AzureOpenAIProxy.PlaygroundApp/Program.cs Outdated Show resolved Hide resolved
@5jisoo 5jisoo force-pushed the feature/225-system-message-params-tab branch from 0f053e2 to fee543f Compare August 30, 2024 16:18
justinyoo
justinyoo requested changes Aug 31, 2024
View reviewed changes
Copy link
Contributor

@justinyoo justinyoo left a comment

Choose a reason for hiding this comment

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

테스트 메소드 구현 방식과 이벤트 핸들러 메소드 네이밍 쪽을 한 번 더 고민해 주세요.

test/AzureOpenAIProxy.PlaygroundApp.Tests/UI/ConfigTabComponentTest.cs Outdated Show resolved Hide resolved
src/AzureOpenAIProxy.PlaygroundApp/Components/Pages/Playground.razor Outdated Show resolved Hide resolved
src/AzureOpenAIProxy.PlaygroundApp/Components/UI/ConfigTabComponent.razor Outdated Show resolved Hide resolved
@5jisoo
Copy link
Contributor Author

5jisoo commented Aug 31, 2024

변경

  1. 테스트 케이스 사용해서 두 탭 모두 테스트할 수 있도록 코드를 변경하였습니다.
  2. HandleOnTabChange -> ChangeTab 네이밍 변경, async Task로 처리하였습니다.

justinyoo
justinyoo approved these changes Aug 31, 2024
View reviewed changes
Copy link
Contributor

@justinyoo justinyoo left a comment

Choose a reason for hiding this comment

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

좋습니다! LGTM!

@justinyoo justinyoo merged commit d7aab48 into aliencube:main Aug 31, 2024
1 check passed
jhmin99 pushed a commit to jhmin99/azure-openai-sdk-proxy that referenced this pull request Sep 1, 2024
@5jisoo @jhmin99
[Playground] Component - Tab for system message and parameters (UI on…
e5273a5 
…ly) aliencube#225 (aliencube#244)
@5jisoo 5jisoo deleted the feature/225-system-message-params-tab branch September 7, 2024 01:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@justinyoo justinyoo justinyoo approved these changes

Assignees

@5jisoo 5jisoo

Labels
ossca playground task team-oh
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Playground] Component - Tab for system message and parameters (UI only)
2 participants
@5jisoo @justinyoo