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

Candidate invitations details#164 #192

Merged
merged 9 commits into from
Sep 21, 2022

Conversation

ice1080
Copy link
Collaborator

@ice1080 ice1080 commented Sep 15, 2022

@ddfridley this one should fix the last two tasks of #164 as mentioned. Because I'm unable to post recordings locally I was unable to fully test this, so it should probably be tested on your side locally before merging.
Let me know if there are any changes needed. Thanks!

@ice1080 ice1080 requested a review from ddfridley September 15, 2022 04:39
@ice1080 ice1080 mentioned this pull request Sep 15, 2022
8 tasks
@ddfridley
Copy link
Contributor

@ice1080 I made a change and pushed it. The invites that was being passed to InviteSentComponent is an object of object, not an array.

But now I have a new challenge, after the candidate has recorded, it still says "Last Sent 1 days ago" so I think it should say submitted. But at that point we are duplicating the status column on the Submissions page. (And on that page, the sent stats just says sent and doesn't say the days ago". Is there an easy way to improve the Submission Status column logic and then use it in both places?

@ice1080
Copy link
Collaborator Author

ice1080 commented Sep 16, 2022

I remember looking into that and it not being very straightforward. If I remember correctly, one of the pages renders the component without passing any data to the render, so it would require quite a bit of refactoring to line them up. In the issue (#164) I had mentioned this and you said if we're going to be combining the pages eventually then it's less of a big deal?
#164 (comment)
#164 (comment)

@ice1080
Copy link
Collaborator Author

ice1080 commented Sep 16, 2022

Also, regarding the change you made, it seems like iotas.json is getting further and further out of date. I definitely recognize that I should have looked at the wiki instead of assuming that file was true, but I recommend making that file line up with the wiki's structure so that future contributors don't make similar mistakes

@ice1080
Copy link
Collaborator Author

ice1080 commented Sep 20, 2022

@ddfridley just tested with my cloudinary url and I see everything working as expected now, including Post recording. This PR is ready for merging now unless you know of anything else you'd like merged in this.

Copy link
Contributor

@ddfridley ddfridley left a comment

Choose a reason for hiding this comment

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

Looks great

@ddfridley ddfridley merged commit 6ac869a into EnCiv:main Sep 21, 2022
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