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.

Sign up for GitHub

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

Jump to bottom

Export company interests to csv #3664

Merged
merged 2 commits into from Mar 25, 2023
Merged

Export company interests to csv #3664

merged 2 commits into from Mar 25, 2023

Conversation

Arashfa0301
Copy link
Contributor

@Arashfa0301 Arashfa0301 commented Mar 10, 2023 •
edited by ivarnakken

Description

  • Implemented filtering company interests by event type, one similar to the filtering by semester.
  • Implemented interface for exporting company interests to csv file. The exporting is filtered by semester and event type
  • The backend implementation was done in here: #3231

Result

image

image

# Testing
  • I have thoroughly tested my changes.

@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 Mar 10, 2023
@Arashfa0301 Arashfa0301 force-pushed the export-companyInterests-to-csv branch 2 times, most recently from 6d18564 to 5e3b57b Compare March 10, 2023 16:21
@Arashfa0301 Arashfa0301 self-assigned this Mar 10, 2023
ivarnakken
ivarnakken previously requested changes Mar 11, 2023
View reviewed changes
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.

Looks good 💯

Most importantly just change the company interest action.

app/reducers/companyInterest.ts Outdated Show resolved Hide resolved
app/routes/companyInterest/CompanyInterestListRoute.ts Outdated Show resolved Hide resolved
app/routes/companyInterest/components/CompanyInterestList.tsx Outdated Show resolved Hide resolved
app/routes/companyInterest/components/CompanyInterestList.tsx Outdated Show resolved Hide resolved
app/routes/companyInterest/CompanyInterestListRoute.ts Outdated Show resolved Hide resolved
@ivarnakken ivarnakken added the changes-requested Pull requests with requested changes label Mar 12, 2023
@Arashfa0301 Arashfa0301 force-pushed the export-companyInterests-to-csv branch from 5e3b57b to 230c7e5 Compare March 13, 2023 12:50
@Arashfa0301 Arashfa0301 force-pushed the export-companyInterests-to-csv branch from 230c7e5 to 92ad4f5 Compare March 14, 2023 12:23
norbye
norbye reviewed Mar 14, 2023
View reviewed changes
Copy link
Member

@norbye norbye left a comment

Choose a reason for hiding this comment

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

These comments are mostly knitpickery, so see if you want to do anything about it - the remaining comments from the others seem more important:)

app/routes/companyInterest/CompanyInterestListRoute.ts Outdated Show resolved Hide resolved
app/routes/companyInterest/CompanyInterestListRoute.ts Outdated Show resolved Hide resolved
app/routes/companyInterest/CompanyInterestListRoute.ts Outdated Show resolved Hide resolved
app/routes/companyInterest/components/CompanyInterestList.tsx Outdated Show resolved Hide resolved
app/routes/companyInterest/components/CompanyInterestList.tsx Outdated Show resolved Hide resolved
@Arashfa0301 Arashfa0301 force-pushed the export-companyInterests-to-csv branch 3 times, most recently from d754891 to 3780ce0 Compare March 15, 2023 21:07
@Arashfa0301 Arashfa0301 force-pushed the export-companyInterests-to-csv branch from 3780ce0 to 2f8486a Compare March 15, 2023 22:27
PeterJFB
PeterJFB approved these changes Mar 16, 2023
View reviewed changes
Copy link
Contributor

@PeterJFB PeterJFB left a comment

Choose a reason for hiding this comment

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

Love the added tooltip 😍 LGTM^^

LudvigHz
LudvigHz approved these changes Mar 16, 2023
View reviewed changes
Copy link
Member

@LudvigHz LudvigHz left a comment

Choose a reason for hiding this comment

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

The only major thing I would comment on is to use replace rather than push. It makes the user experience a lot better.

Other than that it looks really good now 👌

app/routes/companyInterest/components/CompanyInterestList.tsx Show resolved Hide resolved
app/routes/companyInterest/components/CompanyInterestList.tsx Outdated Show resolved Hide resolved
app/routes/companyInterest/components/CompanyInterestList.tsx Outdated Show resolved Hide resolved
app/routes/companyInterest/components/CompanyInterestList.tsx Show resolved Hide resolved
app/routes/companyInterest/components/CompanyInterestList.tsx
Comment on lines +136 to +142
filename: `company-interests-${this.props.selectedSemesterOption.year}-${
this.props.selectedSemesterOption.semester
}${
this.props.selectedEventOption.value
? `-${this.props.selectedEventOption.value}`
: ''
}.csv`,
Copy link
Member

Choose a reason for hiding this comment

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

Don't you already create the filename in the backend? Why not use that instead here?
Not a huge deal but just a bit unnecessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Truthfully, I didn't really understand the point of having it in the back-end. U have to have this here in the front-end for the purpose of changing url blobs.

app/routes/companyInterest/components/CompanyInterestList.tsx Outdated Show resolved Hide resolved
ivarnakken
ivarnakken reviewed Mar 16, 2023
View reviewed changes
app/routes/companyInterest/components/CompanyInterestList.tsx
) : (
<Tooltip
style={
this.props.selectedSemesterOption.year && { display: 'none' }
Copy link
Member

Choose a reason for hiding this comment

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

I feel like adding a display: none isn't very "reacty". Just use !this.props.selectedSemesterOption.year to determine if the JSX should be rendered.

{!this.props.selectedSemesterOption.year && (
    <Tooltip>
	...
    </Tooltip>
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that would remove the children when the predicate is false. How would you avoid that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fixed it. Not sure if this way is better though

app/routes/companyInterest/components/CompanyInterestList.tsx Show resolved Hide resolved
@Arashfa0301 Arashfa0301 force-pushed the export-companyInterests-to-csv branch from 2f8486a to 763e5a6 Compare March 23, 2023 16:22
LudvigHz
LudvigHz reviewed Mar 24, 2023
View reviewed changes
app/routes/companyInterest/components/CompanyInterestList.tsx Outdated
Comment on lines 272 to 278
) : !this.props.selectedSemesterOption.year ? (
<Tooltip content={'Vennligst velg semester'}>
<this.EportButton />
</Tooltip>
) : (
<this.EportButton />
)}
Copy link
Member

Choose a reason for hiding this comment

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

I don't get it 🤔. It tells you to select a semester but you can still click the button right? I'm confused, what's the idea here? 😅

PS: EportButton -> ExportButton

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The button isn't clickable when the tooltip is showing.

Copy link
Member

Choose a reason for hiding this comment

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

Ah I see now.

I get @ivarnakken's point, but if this is the alternative than I'm pretty sure we all agree the previous solution was better 🙃

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Arashfa0301 Arashfa0301 force-pushed the export-companyInterests-to-csv branch from 763e5a6 to 996cdb6 Compare March 25, 2023 15:52
@Arashfa0301 Arashfa0301 force-pushed the export-companyInterests-to-csv branch from 996cdb6 to 89a4a5f Compare March 25, 2023 16:24
@Arashfa0301 Arashfa0301 dismissed ivarnakken’s stale review March 25, 2023 16:25

I have fixed all these now

@Arashfa0301 Arashfa0301 merged commit c7b6a2b into master Mar 25, 2023
2 checks passed
@Arashfa0301 Arashfa0301 deleted the export-companyInterests-to-csv branch March 25, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@norbye norbye norbye left review comments

@PeterJFB PeterJFB PeterJFB approved these changes

@LudvigHz LudvigHz LudvigHz approved these changes

@erlingfn erlingfn Awaiting requested review from erlingfn

@eikhr eikhr Awaiting requested review from eikhr

@SmithPeder SmithPeder Awaiting requested review from SmithPeder

@AdrianAndersen AdrianAndersen Awaiting requested review from AdrianAndersen

@juniwbjerde juniwbjerde Awaiting requested review from juniwbjerde

@ollfkaih ollfkaih Awaiting requested review from ollfkaih

@ivarnakken ivarnakken Awaiting requested review from ivarnakken

Assignees

@Arashfa0301 Arashfa0301

Labels
changes-requested Pull requests with requested changes 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
Milestone
No milestone
5 participants
@Arashfa0301 @norbye @PeterJFB @LudvigHz @ivarnakken