-
Notifications
You must be signed in to change notification settings - Fork 495
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
Groups export all posts #4657
base: master
Are you sure you want to change the base?
Groups export all posts #4657
Conversation
…in student view to address issue cpinitiative#2285
for more information, see https://pre-commit.ci
oh my god an actual web dev pr |
This pull request has been automatically marked as stale because it has not had recent activity. Please address the requested changes and re-request reviews. Thank you for your contribution! |
@jayana-cpc sent you a message on discord, let's review this together when you have a chance |
This pull request has been automatically marked as stale because it has not had recent activity. Please address the requested changes and re-request reviews. Thank you for your contribution! |
This pull request has been automatically marked as stale because it has not had recent activity. Please address the requested changes and re-request reviews. Thank you for your contribution! |
This pull request has been automatically marked as stale because it has not had recent activity. Please address the requested changes and re-request reviews. Thank you for your contribution! |
This pull request has been automatically marked as stale because it has not had recent activity. Please address the requested changes and re-request reviews. Thank you for your contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
overall lgtm, thanks for implementing this!
from my brief testing it seems like the posts are imported out-of-order, which is not ideal but not blocking.
<button | ||
type="button" | ||
className="block w-full text-left px-4 py-2 text-sm text-gray-700 hover:bg-gray-100 hover:text-gray-900 focus:outline-none focus:bg-gray-100 focus:text-gray-900" | ||
onClick={handleExportAllPosts} | ||
> | ||
Export All Posts | ||
</button> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should only show this for owners of the group you're exporting from?
groups.data.map(group => | ||
group && | ||
group.ownerIds.includes(firebaseUser!.uid) ? ( | ||
<div key={group.id}> | ||
<label className="inline-flex items-center"> | ||
<input | ||
type="checkbox" | ||
className="form-checkbox" | ||
onChange={() => | ||
handleGroupExportChange(group) | ||
} | ||
/> | ||
<span className="ml-2 text-gray-500"> | ||
{group.name} | ||
</span> | ||
</label> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor, but I think right now it will allow you to export a group to itself, which probably isn't desireable.
This pull request has been automatically marked as stale because it has not had recent activity. Please address the requested changes and re-request reviews. Thank you for your contribution! |
This pull request has been automatically marked as stale because it has not had recent activity. Please address the requested changes and re-request reviews. Thank you for your contribution! |
Place an "x" in the corresponding checkbox if it is done or does not apply to this pull request.