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

fix: deleting workspace now makes the name disappear #1243

Conversation

sebp999
Copy link
Contributor

@sebp999 sebp999 commented Nov 4, 2021

Description

Fixes #1014: alter static/saving/game/saving.js to make the level name disappear after successful delete

How Has This Been Tested?

Manual test (Chrome v95) and ran unit tests

Screenshots (if appropriate):

Checklist:

  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • [ √ ] I have linked this PR to a Zenhub Issue.

This change is Reviewable

Copy link
Collaborator

@razvan-pro razvan-pro left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @sebp999)


game/static/game/js/saving.js, line 95 at r1 (raw file):

            },
            success: function (json) {
                $("#loadWorkspaceTable > tr[selected]").remove();

Hi @sebp999! This fixes it when there are multiple rows in the table, but when it's just one, I see the table will look like this until refreshed:

image.png

Also, I think the workspace remains selected after deleting it and the buttons are active as well, so clicking on "Delete" again will trigger an error - this happens when there are multiple rows too.

Can you have a look at fixing those as well, please? (From a quick look, loadInWorkspaces on line 970 in game.js seems to be useful - maybe we can call this function with changed workspaces to fix the one-row case and active buttons too? Otherwise, just copying some bits from it will do (or better even, extracting them into a separate function))

@sebp999
Copy link
Contributor Author

sebp999 commented Apr 22, 2022

Hi @razvan-pro thanks for the pointer. I made another change which fixes it. (Turns out there was an undefined this). Have a look.

Copy link
Collaborator

@razvan-pro razvan-pro left a comment

Choose a reason for hiding this comment

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

Yep, this works, thanks @sebp999!

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @sebp999)

@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #1243 (1f2a5f9) into master (9a4c2e8) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1243   +/-   ##
=======================================
  Coverage   91.52%   91.52%           
=======================================
  Files          99       99           
  Lines        5296     5296           
=======================================
  Hits         4847     4847           
  Misses        449      449           
Impacted Files Coverage Δ
game/random_road.py 91.00% <0.00%> (ø)

@razvan-pro razvan-pro merged commit 4464139 into ocadotechnology:master May 24, 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.

Deleting workspace doesn't have any feedback and causes error
3 participants