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

Issue 1553 #1555

Merged
merged 10 commits into from Oct 30, 2019

Conversation

@c3ho
Copy link
Contributor

c3ho commented Oct 23, 2019

Hi, this is the PR for issue-1553.

A few things I did:

  • removed the button to materialize creatures when viewing a creature. Only right clicking on an empty hex or the godlet printer will display the materialize button
  • I commented a ton of lines out, was a bit afraid to remove them, if requested I can since they'll be displayed in the commit
  • added two new parameters to showCreature, although I'm pretty sure the first one is not needed, now that we don't need to keep track of the creature being viewed and the creature we want to summon(which needed to be tracked when right clicking on a creature on the field). The second one lets showCreature know to disable the materialize button if we are right clicking on a unit from the field
@now

This comment has been minimized.

Copy link

now bot commented Oct 23, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

@now now bot had a problem deploying to staging Oct 23, 2019 Failure
@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 23, 2019

@DreadKnight give this a try and see if the changes are to your liking. I can clean up some of the functions afterwards!

@DreadKnight DreadKnight temporarily deployed to ancientbeast-issue-1553-geikw0 Oct 25, 2019 Inactive
@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 25, 2019

@c3ho I've tested this and works fine, hope you'll clean up the stuff a little bit; I've found a few other bugs in the process while testing this, new issues opened xD

@now now bot had a problem deploying to staging Oct 26, 2019 Failure
@DreadKnight DreadKnight temporarily deployed to ancientbeast-issue-1553-geikw0 Oct 26, 2019 Inactive
@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 26, 2019

@c3ho This is full of problems now, you need to test it more. The materialize button shows up a lot!

@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 26, 2019

@DreadKnight I’ll double check, might’ve pushed the wrong branch. Thanks!

@now now bot had a problem deploying to staging Oct 26, 2019 Failure
@DreadKnight DreadKnight temporarily deployed to ancientbeast-issue-1553-geikw0 Oct 26, 2019 Inactive
@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 26, 2019

@DreadKnight
Tested the commit thoroughly this time =D. materialize button should appear a lot less now.
Materialize Button will not appear if:

  • the active creature is not a Dark Priest
  • the Dark Priest has already used materialize for the turn
  • right clicking on a hex with a summoned creature

Materialize Button will appear if:

  1. the active creature is a Dark Priest and has not summoned a creature for the turn
  2. Condition 1 is true and if player right clicked on a hex without a creature
@now now bot had a problem deploying to staging Oct 26, 2019 Failure
@DreadKnight DreadKnight temporarily deployed to ancientbeast-issue-1553-geikw0 Oct 26, 2019 Inactive
@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 26, 2019

@c3ho Found a bug: after materializing an unit, right clicking on an empty space still shows the button.

@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 26, 2019

@DreadKnight Oh I see.

@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 26, 2019

@DreadKnight were there other instances where you saw the materialize button appear when it shouldn't?

@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 26, 2019

@c3ho Did more testing and found another one, by clicking on avatars from the unit queue located top.

@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 26, 2019

@DreadKnight thanks, I think I just need to fix the clicking on portraits now that's left. Thank you for putting up with me haha.

@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 26, 2019

@c3ho No worries, it's always awesome when people like you take the time to contribute to the project!

@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 27, 2019

@DreadKnight When clicking on the portrait of the creatures, should the materialize bar appear if the player clicks on the portrait of their Dark Priest, or should it behave just like right clicking on a hex with a creature on it?

@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 27, 2019

@c3ho The logic is that when active unit is Dark Priest, the materialization button should guide the player into finding a suitable creature to materialize, hence why the status/error strings for that button. When not Dark Priest's turn, materialization button should be hidden 100%.

@now now bot had a problem deploying to staging Oct 29, 2019 Failure
@DreadKnight DreadKnight temporarily deployed to ancientbeast-issue-1553-geikw0 Oct 29, 2019 Inactive
@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 29, 2019

@DreadKnight fixed the portrait issue as well. Materialize button should only appear if and only it is their Dark Priest's turn AND the materialize ability has not been used for the turn yet.

I think there's is going to be at least 1 maybe 2 bug(s) that need to be filed.

  1. If a player views any creature from the Godlet Printer, but doesn't materialize, and clicks on the portrait of their own Dark Priest, the materialize button text will display with the plasma cost of the last viewed creature even though the Dark Priest is displayed.

  2. Sometimes after clicking on the portrait and clicking to another creature after the materialize ability has been used, the message "This unit is under heavy development" appears.

@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 29, 2019

@c3ho Indeed, also:
3 . After materializing a unit and checking out the dash again, an error message should be displayed under the cards of available (non dead and non queued) creatures, as your PR makes "Materialization has already been used this round" string not shown anymore

@now now bot had a problem deploying to staging Oct 30, 2019 Failure
@DreadKnight DreadKnight temporarily deployed to ancientbeast-issue-1553-geikw0 Oct 30, 2019 Inactive
@c3ho c3ho force-pushed the c3ho:issue-1553 branch from 8b8e858 to 6d765ca Oct 30, 2019
@now now bot had a problem deploying to staging Oct 30, 2019 Failure
@DreadKnight DreadKnight temporarily deployed to ancientbeast-issue-1553-geikw0 Oct 30, 2019 Inactive
@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 30, 2019

@DreadKnight sorry, mistakenly rebased with a commit from the master branch. Had to drop the commit and re-push!

Let me know how it goes. For some reason I got a bug when trying to summon the creature Stomper.

@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 30, 2019

@c3ho No worries, we dealt with some extra pipeline issues, had a temporary workaround committed for now. Regarding Stomper and a few others, they're not yet playable. See this regarding their "schedule" https://github.com/FreezingMoon/AncientBeast/projects/3
Stomper is extra problematic atm just by selecting the card itself #1563

If you're not poking at the remaining issues, I can merge and open up new issues in the tracker.

@now now bot had a problem deploying to staging Oct 30, 2019 Failure
@DreadKnight DreadKnight temporarily deployed to ancientbeast-issue-1553-geikw0 Oct 30, 2019 Inactive
@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 30, 2019

Alright this one should be able to be merged. If the Godlet Printer has been used in the same turn, The Godlet Printer will show the 'Materialization has already been used this round' when a player re-opens dash, but if a player clicked on another creature in the grid afterwards the materialization bar disappeared. This last commit was to fix that issue.

@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 30, 2019

@c3ho I don't get why your package-lock.json file is committed xD
Materialization has already been used this round shouldn't be showing for queued or dead units btw, but I'll make that a separate issue.

@c3ho

This comment has been minimized.

Copy link
Contributor Author

c3ho commented Oct 30, 2019

@DreadKnight I committed the file because the deployment thing kept bothering me about it. Sorry!

@now now bot had a problem deploying to staging Oct 30, 2019 Failure
@DreadKnight DreadKnight temporarily deployed to ancientbeast-issue-1553-geikw0 Oct 30, 2019 Inactive
@DreadKnight DreadKnight merged commit 54c4f78 into FreezingMoon:master Oct 30, 2019
1 check failed
1 check failed
now Deployment has failed
Details
@DreadKnight

This comment has been minimized.

Copy link
Member

DreadKnight commented Oct 30, 2019

@c3ho No worries. Well done with the issue!
I've opened #1566 regarding that error button and will see about the rest as well soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.