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

Fix the UI user flow of selecting custom teanant on tenant switch panel #1112

Merged
merged 1 commit into from
Oct 4, 2022

Conversation

RyanL1997
Copy link
Collaborator

@RyanL1997 RyanL1997 commented Sep 29, 2022

Signed-off-by: Ryan Liang [email protected]

Description

  • Add description into the custom tenant selecting dropdown list, and disable the button when there is no valid selection for custom tenant.
  • Update the unit test.

Category

Bug fix

Issues Resolved

Testing

Sep-30-2022 12-13-31

Check List

  • New functionality includes testing
  • New functionality has been documented
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@RyanL1997 RyanL1997 force-pushed the bugfix819 branch 4 times, most recently from 29e5233 to 79a3020 Compare September 30, 2022 03:39
@codecov-commenter
Copy link

codecov-commenter commented Sep 30, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 74.43%. Comparing base (bf5691f) to head (f27aafc).
Report is 208 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1112      +/-   ##
==========================================
+ Coverage   74.39%   74.43%   +0.04%     
==========================================
  Files          86       86              
  Lines        1871     1874       +3     
  Branches      241      244       +3     
==========================================
+ Hits         1392     1395       +3     
  Misses        424      424              
  Partials       55       55              

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

@RyanL1997 RyanL1997 marked this pull request as ready for review September 30, 2022 13:49
@RyanL1997 RyanL1997 requested a review from a team September 30, 2022 13:49
@RyanL1997
Copy link
Collaborator Author

Hi @shanilpa, thank you for the instruction: #819 (comment). The fix has been implemented.

peternied
peternied previously approved these changes Sep 30, 2022
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

Looks fine to me, might need some work to get the CI working

@RyanL1997
Copy link
Collaborator Author

RyanL1997 commented Sep 30, 2022

Looks fine to me, might need some work to get the CI working

Hi @peternied, thanks for the review. I just fixed the tests.

Copy link
Member

@DarshitChanpura DarshitChanpura left a comment

Choose a reason for hiding this comment

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

Thanks @RyanL1997 !

@@ -297,8 +298,18 @@ export function TenantSwitchPanel(props: TenantSwitchPanelProps) {

<EuiButton
data-test-subj="confirm"
fill
disabled={!isMultiTenancyEnabled}
fill={
Copy link
Member

Choose a reason for hiding this comment

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

Can we move the logic into a local variable it is not duplicated it on both lines?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the review! I just made an update on this.

@@ -297,8 +301,8 @@ export function TenantSwitchPanel(props: TenantSwitchPanelProps) {

<EuiButton
data-test-subj="confirm"
fill
disabled={!isMultiTenancyEnabled}
fill={isMultiTenancyEnabled && !invalidCustomTenant}
Copy link
Member

Choose a reason for hiding this comment

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

I see we've now added a condition on fill here. I just want to confirm if this is intended behavior.

fill is related to whether the bg of the button is filled in or transparent. See https://eui.elastic.co/pr_3350/#/navigation/button

Screen Shot 2022-10-03 at 10 31 57 AM

Copy link
Collaborator Author

@RyanL1997 RyanL1997 Oct 3, 2022

Choose a reason for hiding this comment

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

Hi @cwperks, thanks for the review! I talked to @shanilpa, and he suggested me to change the disabled button to the ghosted disabled without the fill option, because that's the pattern we are moving forward with. That's why I added the condition to the fill option.

Copy link

Choose a reason for hiding this comment

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

@RyanL1997 that's correct. We're looking to reduce some of the visual clutter by making use of the ghosted disabled state. Thanks for the review @cwperks.

peternied
peternied previously approved these changes Oct 3, 2022
cwperks
cwperks previously approved these changes Oct 4, 2022
@cwperks
Copy link
Member

cwperks commented Oct 4, 2022

@RyanL1997 Looks like this branch needs to be re-based, please re-base with the latest changes in main. Should this change be backported to 2.x?

@RyanL1997
Copy link
Collaborator Author

@RyanL1997 Looks like this branch needs to be re-based, please re-base with the latest changes in main. Should this change be backported to 2.x?

@cwperks Thanks! I just re-pushed with the rebased repo. Yeah, I think we do need to add the 2.x backporting label.

@cwperks cwperks added the backport 2.x backport to 2.x branch label Oct 4, 2022
@peternied peternied merged commit 3bfd7c5 into opensearch-project:main Oct 4, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 4, 2022
…el (#1112)

Signed-off-by: Ryan Liang <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
(cherry picked from commit 3bfd7c5)
cwperks pushed a commit that referenced this pull request Oct 4, 2022
…el (#1112) (#1117)

Signed-off-by: Ryan Liang <[email protected]>
Signed-off-by: Ryan Liang <[email protected]>
(cherry picked from commit 3bfd7c5)

Co-authored-by: Ryan Liang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No warning or error message displays when no tenant is chosen under choose from custom option
6 participants