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

[#10569] Missing loading icons and bugs for session edit form #11007

Conversation

jfrairigh
Copy link
Contributor

@jfrairigh jfrairigh commented Mar 5, 2021

Part of #10569

Outline of Solution

I found the HTML page for the session edit form and added directives to each button to turn the spinner component on and off based on boolean values. I then went to methods of the corresponding action (delete, cancel, etc.) and boolean variables to determine if the action was still in progress. The top save button was slightly different. Someone had already added the directive. However, the save button was being deleted before the save processed had finished. I changed the HTML code to ensure the button remained throughout the saving processes.

@moziliar
Copy link
Contributor

moziliar commented Mar 5, 2021

Hi, can you show some screenshot of the before/after change view? This way, it is easier to view the direct impact of the intended changes.

@madanalogy madanalogy added the s.Ongoing The PR is being worked on by the author(s) label Mar 9, 2021
@jfrairigh
Copy link
Contributor Author

@jfrairigh
Copy link
Contributor Author

@moziliar This should be ready for review now. Let me know if you need anything else from me.

Copy link
Contributor

@moziliar moziliar left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution!

I am not sure if this PR is linked to the right issue/PR as I cannot elicit any context from the target page. Could you check it again? Otherwise, could you fill me in with some context?

I think the spinners should be great to have.

However, the save button was being deleted before the save processed had finished.

Maybe add a slower version of this scene could be snipped for reference? The video doesn't show a UX flaw induced by this effect.

@moziliar moziliar added s.ToReview The PR is waiting for review(s) and removed s.Ongoing The PR is being worked on by the author(s) labels Mar 11, 2021
@jfrairigh jfrairigh force-pushed the 10569-Missing-Loading-Icons-Instructor-Session-Edit branch from 0eb82f1 to 5e52c11 Compare March 11, 2021 20:12
Copy link
Contributor

@moziliar moziliar left a comment

Choose a reason for hiding this comment

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

Do revert the package-lock.json file that is modified, probably due to an npm install that updates the hash.
Otherwise, this PR LGTM!

@jfrairigh jfrairigh force-pushed the 10569-Missing-Loading-Icons-Instructor-Session-Edit branch from 2480e6f to ac16d86 Compare March 12, 2021 06:44
Copy link
Contributor

@moziliar moziliar left a comment

Choose a reason for hiding this comment

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

LGTM. The changes are clean.

@moziliar moziliar added s.FinalReview The PR is ready for final review and removed s.ToReview The PR is waiting for review(s) labels Mar 12, 2021
@moziliar
Copy link
Contributor

Some tips about the revert process is that:
You could perform git reset --soft HEAD^, git reset, git add <only the necessary files>, git commit, git push -f to overwrite the last commit you had. In this case, I think it'd be squashed so it would not matter.

@jfrairigh
Copy link
Contributor Author

Thank you for the advice on the revert process. I will keep that in mind for next time. Sorry for the excessive commits.

@moziliar
Copy link
Contributor

Do note that try not to overwrite the history that is in used elsewhere, e.g. in collab branch or the branch is in use elsewhere, in which case a revert commit is a wiser choice, albeit the extra commit.

Copy link
Contributor

@madanalogy madanalogy left a comment

Choose a reason for hiding this comment

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

LGTM

@madanalogy madanalogy added s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging c.Feature User-facing feature; can be new feature or enhancement to existing feature and removed s.FinalReview The PR is ready for final review labels Mar 13, 2021
@madanalogy madanalogy added this to the V7.12.0 milestone Mar 13, 2021
@madanalogy madanalogy merged commit 4f8fc04 into TEAMMATES:master Mar 14, 2021
@jfrairigh jfrairigh deleted the 10569-Missing-Loading-Icons-Instructor-Session-Edit branch March 14, 2021 18:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c.Feature User-facing feature; can be new feature or enhancement to existing feature s.ToMerge The PR is approved by all reviewers including final reviewer; ready for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants