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

Refactor/companies dynamic #46

Merged
merged 8 commits into from
Jan 7, 2025
Merged

Conversation

jimmacur
Copy link
Collaborator

@jimmacur jimmacur commented Jan 6, 2025

Type of Change

  • refactor πŸ§‘β€πŸ’»
  • testing πŸ§ͺ

Description

Overview

This PR introduces improved dynamic data handling for the Companies and Contacts modules. Updates include API-driven rendering, refactored Cypress tests for dynamic data, and UI enhancements for a better user experience.


Key Changes

Dynamic Contacts Integration

  • Contacts are fetched dynamically via the /companies/:id/contacts API endpoint.
  • Updated CompanyShow component to display fetched contact data.

Dynamic Job Applications Integration

  • Job applications are dynamically fetched and mapped to companies, displaying statuses in the Companies table.

Enhanced Cypress Tests

  • Tests for the Companies and Company Show pages are updated to validate API-driven dynamic data.
  • New scenarios added to test empty states, dynamic data rendering, and navigation.

Form Enhancements

  • Placeholder text added to NewCompany form fields for better usability.
  • Validation updates to allow submission with only required fields (company name).

Cypress Test Refactoring

  • Updated test cases to use dynamic userId for intercepts.
  • Verified UI updates for dynamic contacts and application statuses.
  • Updated intercepts to include - Job Applications: GET /users/:id/job_applications

Motivation and Context

This fulfills the FE portion of #65

Added Test?

  • Yes 🫑
  • All previous tests still pass πŸ₯³

Checklist:

  • My code follows the code style of this project.

Copy link
Collaborator

@stefanjbloom stefanjbloom 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 a merge conflict that needs to be resolved, then ready to merge. Readable code and well done with the cypress tests.

application_url: string;
contact_information: string;
company_id: number;
company_name?: string;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

The usage of an Interfaces file(and an api fetch file since it's the same idea) for all component usage is mega SRP. Perhaps an issue can be made to migrate all interfaces to this file?

{companyName}
</td>
<td className="p-4 border-b truncate max-w-[8vw]">{data.attributes.notes}</td>
</tr>
)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This addition/subtraction looks identical, unless I am mistaken. Nonetheless it makes sense.

@stefanjbloom stefanjbloom merged commit a463f5a into main Jan 7, 2025
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.

2 participants