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

fixed camel case? #60

Merged
merged 10 commits into from
Sep 3, 2021
Merged

fixed camel case? #60

merged 10 commits into from
Sep 3, 2021

Conversation

DerekL27
Copy link
Contributor

@DerekL27 DerekL27 commented Aug 2, 2021

Why

Ticket
Ticket

This PR

Fixes camel case? Not sure if this is what the ticket meant

@coveralls
Copy link

Pull Request Test Coverage Report for Build 1091798901

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 17.037%

Totals Coverage Status
Change from base Build 1049750552: 0.0%
Covered Lines: 250
Relevant Lines: 1250

💛 - Coveralls

@coveralls
Copy link

coveralls commented Aug 2, 2021

Pull Request Test Coverage Report for Build 1164376598

  • 1 of 23 (4.35%) changed or added relevant lines in 3 files are covered.
  • 21 unchanged lines in 2 files lost coverage.
  • Overall coverage decreased (-0.07%) to 16.947%

Changes Missing Coverage Covered Lines Changed/Added Lines %
src/utils/helpers.tsx 1 3 33.33%
src/components/userDirectory/CreateUser.tsx 0 3 0.0%
src/containers/userDirectory/index.tsx 0 17 0.0%
Files with Coverage Reduction New Missed Lines %
src/containers/pastSubmissionsReports/PastSubmissionReports.tsx 7 0%
src/containers/userDirectory/index.tsx 14 0%
Totals Coverage Status
Change from base Build 1116631719: -0.07%
Covered Lines: 251
Relevant Lines: 1260

💛 - Coveralls

@justinkonecny
Copy link
Contributor

Screenshots?

Did you test this to make sure it doesn't break anything?

@granttebeau
Copy link
Collaborator

Screen Shot 2021-08-03 at 11 24 05 AM

Still have camel case here for the country and privilege columns, we'll need to make these normal case as well in the user directory

@DerekL27
Copy link
Contributor Author

DerekL27 commented Aug 3, 2021

image

@justinkonecny
@granttebeau

should be fixed now

@DerekL27 DerekL27 requested a review from granttebeau August 3, 2021 23:10
Copy link
Collaborator

@granttebeau granttebeau left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@maxsebso maxsebso left a comment

Choose a reason for hiding this comment

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

The problem we had last time was that the form item would now contain no camel case. This would mean the request would come back bad request because the backend would be expecting camel case. If you are sure that you tested this thoroughly I will approve. If it is not tested it can and will break the whole form.

@DerekL27
Copy link
Contributor Author

The problem we had last time was that the form item would now contain no camel case. This would mean the request would come back bad request because the backend would be expecting camel case. If you are sure that you tested this thoroughly I will approve. If it is not tested it can and will break the whole form.

Yeah you're right it did break the form. I changed it so that it takes the enum and formats it to look nice so now everything should be good. I tested it and it works

# Conflicts:
#	src/components/userDirectory/CreateUser.tsx
@DerekL27 DerekL27 requested a review from maxsebso August 24, 2021 21:15
Copy link
Contributor

@maxsebso maxsebso left a comment

Choose a reason for hiding this comment

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

Almost! Just one thing. Since you are using the function convertEnumToRegularText 3 times in 3 different files you should put it in utils folder and then import it when it needs to be used

@DerekL27 DerekL27 requested a review from maxsebso August 24, 2021 22:19
@DerekL27
Copy link
Contributor Author

Almost! Just one thing. Since you are using the function convertEnumToRegularText 3 times in 3 different files you should put it in utils folder and then import it when it needs to be used

Smart

Copy link
Member

@rymaju rymaju left a comment

Choose a reason for hiding this comment

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

Small nitpick ⛏️ otherwise LGTM

@@ -19,6 +19,13 @@ export const getOptionsFromEnum = (e: {
));
};

export const convertEnumToRegularText = (input: string) => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
export const convertEnumToRegularText = (input: string) => {
export const convertEnumToRegularText = (input: string): string => {

Copy link
Contributor

@maxsebso maxsebso left a comment

Choose a reason for hiding this comment

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

LGTM

@maxsebso maxsebso changed the base branch from master to Frontend-Staging September 3, 2021 18:34
@maxsebso maxsebso merged commit 426eee0 into Frontend-Staging Sep 3, 2021
@jtavera235 jtavera235 deleted the dl-fix_camel_case branch January 22, 2022 18:51
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.

6 participants