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

Edit company interest #3608

Merged
merged 4 commits into from Feb 22, 2023
Merged

Edit company interest #3608

merged 4 commits into from Feb 22, 2023

Conversation

Arashfa0301
Copy link
Contributor

@Arashfa0301 Arashfa0301 commented Feb 21, 2023

Description

Edited company interest page with long awaited features that were requested by "interkom".

  • Added company survey checkboxes
  • Relocated about company field to the top of the form
  • Added description fields for each event type
  • Added accomodating radio button
    The backend side of all above charges have been implemented and there already exists a pull request for them.
  • And lastly made some minor changes to the styling of check boxes to improve the alignment

Result

after

image

image

Testing

We have thoroughly tested our changes on the webside. We tested that the newly created field and checkboxes work properly and actually push the data to the backend. And also that the newly edited data show up after reopening the form

@Arashfa0301 Arashfa0301 added priority:high Pull requests that have high priority, and should therefore be prioritized review-needed Pull requests that need review new-feature Pull requests that introduce a new feature labels Feb 21, 2023
Copy link
Member

@ivarnakken ivarnakken left a comment

Choose a reason for hiding this comment

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

Some stuff here and there that you should look at - otherwise nice! 💯

app/components/Form/Field.css Outdated Show resolved Hide resolved
Comment on lines 161 to 165
.checkboxSpan {
display: flex;
gap: 4px;
align-items: center;
}
Copy link
Member

Choose a reason for hiding this comment

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

Use the <Flex /> component instead.

Copy link
Member

Choose a reason for hiding this comment

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

And, are you sure it is even needed?

Comment on lines +157 to +159
ion-icon {
font-size: 20px;
}
Copy link
Member

Choose a reason for hiding this comment

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

Use classnames instead of generic tags, as it may affect other icons unexpectedly.

Copy link
Member

Choose a reason for hiding this comment

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

Just use the size prop on the <Icon /> component for this though.

Copy link
Member

Choose a reason for hiding this comment

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

bruh u never fixed this

Copy link
Member

Choose a reason for hiding this comment

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

@Arashfa0301 Arashfa0301 force-pushed the edit-company-interest branch 3 times, most recently from 9977488 to a2a6c8f Compare February 21, 2023 17:53
Copy link
Contributor

@erlingfn erlingfn left a comment

Choose a reason for hiding this comment

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

The schema looks good! A few nitpicks regarding some naming and spelling, but it should be ready soon

app/reducers/companyInterest.ts Outdated Show resolved Hide resolved
app/reducers/companyInterest.ts Outdated Show resolved Hide resolved
app/routes/companyInterest/utils.tsx Outdated Show resolved Hide resolved
app/routes/companyInterest/utils.tsx Outdated Show resolved Hide resolved
app/routes/companyInterest/utils.tsx Outdated Show resolved Hide resolved
app/routes/companyInterest/utils.tsx Outdated Show resolved Hide resolved
app/routes/companyInterest/utils.tsx Outdated Show resolved Hide resolved
Copy link
Member

@eikhr eikhr left a comment

Choose a reason for hiding this comment

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

Good

@eikhr eikhr enabled auto-merge February 21, 2023 23:52
@@ -20,7 +20,7 @@
"test:coverage": "yarn run test -- --coverage",
"test:watch": "yarn run test --watch",
"lint": "yarn run lint:js && yarn run lint:css && yarn run lint:prettier",
"lint:js": "eslint . --ignore-path .prettierignore --max-warnings 1075",
Copy link
Member

Choose a reason for hiding this comment

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

💀

Copy link
Member

Choose a reason for hiding this comment

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

We needed those warnings!

@eikhr eikhr merged commit 41cc668 into master Feb 22, 2023
@eikhr eikhr deleted the edit-company-interest branch February 22, 2023 00:12
@ivarnakken ivarnakken mentioned this pull request Mar 7, 2023
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-feature Pull requests that introduce a new feature priority:high Pull requests that have high priority, and should therefore be prioritized review-needed Pull requests that need review
Projects
None yet
5 participants