Skip to content

Conversation

@pwjablonski
Copy link
Contributor

Hey @outoftime this addresses #1680. I'm not a huge fan of using the spinner again but it was too easy to implement. Also not sure if the exportingAssignment flag should live in the client reducer or the googleClassroom record. Let me know what you think.

@outoftime outoftime self-requested a review April 13, 2019 00:30
Copy link
Contributor

@outoftime outoftime left a comment

Choose a reason for hiding this comment

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

Agreed on the spinner—it is honestly pretty jank looking (which is much more of a problem in this case where you are pretty much guaranteed to see it, as opposed to the original case in which you almost certainly won't see it). Why not just replace the row of buttons with a single button, disabled/with a gray background, that says something like “saving”?

As for where to store the saving state, it’s an interesting question, but I do ultimately think it belongs in the clients namespace rather than googleClassroom. I think the googleClassroom namespace is best thought of as a mirror of actual data stored on Google Classroom, so anything that isn’t data should be kept elsewhere.

Incidentally I think one advantage of moving to something like Formik (which has internal saving state out of the box) would be avoiding such angels-on-the-head-of-a-pin type questions : )

@pwjablonski pwjablonski force-pushed the saving-assignment-indicator branch from e973f2b to 11f1f68 Compare April 16, 2019 20:41
@pwjablonski pwjablonski requested a review from outoftime April 17, 2019 00:28
{t('assignment-creator.creating')}
</button>
</div> :
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use a fragment here! No need for the extra <div> just for JSX structural reasons.

@pwjablonski pwjablonski requested a review from outoftime April 17, 2019 02:45
@outoftime outoftime merged commit 73328fa into popcodeorg:master Apr 17, 2019
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