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

WOR-703 Part 1: changes to existing GCP billing project wizard #3777

Merged
merged 20 commits into from
Feb 13, 2023

Conversation

blakery
Copy link
Contributor

@blakery blakery commented Feb 8, 2023

WOR-703

This PR is the changes the existing step wizard for GCP, in order to extract it into a separate component.
Original PR here

@blakery blakery requested review from cahrens, msilva-broad, a team and marctalbott February 8, 2023 16:05
@blakery blakery mentioned this pull request Feb 8, 2023
@blakery blakery changed the title cherry-picking changes to existing GCP billing project wizard WOR-703 Part 1: changes to existing GCP billing project wizard Feb 8, 2023
@broadbot broadbot temporarily deployed to pr-4 February 8, 2023 20:23 Inactive
@blakery blakery requested a review from cahrens February 9, 2023 14:08
@broadbot broadbot temporarily deployed to pr-4 February 9, 2023 14:17 Inactive
@cahrens
Copy link
Contributor

cahrens commented Feb 9, 2023

@blakery I'm seeing the first text for Step 2 appear to the right of Step 2 instead of below it.

image

@cahrens
Copy link
Contributor

cahrens commented Feb 9, 2023

It looks like we lost the extra space at the bottom of Step 4, which I believe was there to allow the display of error messages. Here it is on prod:

image

@cahrens
Copy link
Contributor

cahrens commented Feb 9, 2023

Test for Step 4 isn't appearing below Step 4, and the button text has changed to all caps (was that intentional?):

image

On prod:

image

@cahrens
Copy link
Contributor

cahrens commented Feb 9, 2023

There is something odd going on with the rendering of the checkbox.

image

@cahrens
Copy link
Contributor

cahrens commented Feb 9, 2023

Since we have the room, can we make the select fields wider so that they don't resize for the very commonly shown message about "Billing Project name is too short"?

image

@cahrens
Copy link
Contributor

cahrens commented Feb 9, 2023

Since the BillingProjectWizard is rapidly becoming less "new", could we just call the directory "BillingProjectWizard"?

Ugh, I just realized that "New" referred to "BillingProject", not "BillingProjectWizard". So nevermind.

@broadbot broadbot temporarily deployed to pr-4 February 10, 2023 19:41 Inactive
@broadbot broadbot temporarily deployed to pr-4 February 13, 2023 16:41 Inactive
@broadbot broadbot temporarily deployed to pr-4 February 13, 2023 18:01 Inactive
Copy link
Member

@marctalbott marctalbott left a comment

Choose a reason for hiding this comment

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

There's currently only tests for the AddTerraAsBillingAccountUserStep, do we need tests for each step? Tested it out locally though and everything looks good

@cahrens
Copy link
Contributor

cahrens commented Feb 13, 2023

There's currently only tests for the AddTerraAsBillingAccountUserStep, do we need tests for each step? Tested it out locally though and everything looks good

The was an extensive pre-existing unit test for the whole wizard that is still running. I'm not sure what @blakery's plan was with regard to testing each individual step in isolation, but we certainly haven't lost any coverage with this PR.

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 2 Code Smells

88.0% 88.0% Coverage
0.0% 0.0% Duplication

@broadbot broadbot temporarily deployed to pr-4 February 13, 2023 21:37 Inactive
@cahrens cahrens merged commit 30a0cc0 into dev Feb 13, 2023
@cahrens cahrens deleted the WOR-703-Part-1 branch February 13, 2023 21:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants