-
Notifications
You must be signed in to change notification settings - Fork 1
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
Bug fixes #113
base: dev
Are you sure you want to change the base?
Bug fixes #113
Conversation
✅ Deploy Preview for test-cheddar-ecosystem ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
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.
In addition to comments made, i found that, even when some texts and styles were changed pointing that after a match the cheddar earned should be not minted inmidiatly and claimed all toghether after clicking a button, this is not happening.
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.
I agree with the changes that Martin asked for.
@Mart-Dan-Rossi @RodrigoCSolari I have resolved the comments, and removed the board related changes (randomness and unreachable cell), so we can merge the other fixed bugs. |
src/components/maze/Gameboard.tsx
Outdated
: 'playerBackgroundElementOnTop'; | ||
|
||
// Combine multiple CSS classes using the 'clsx' utility function to handle conditional styling | ||
return clsx( |
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.
We have multiple lines in the project where we should use this function to be consistent on the way to do this stuff. I know it's not a priority right now to refactor the code but it doesn't look good idea to comment every place where we use this function. I was expecting some better documentation, like creating a local function just to call the clsx but adding a description so it could be seen while hovering the function on the code or something like that
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.
we can do that in refactoring session.
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.
Yeah but if that can be done in refactoring season, ¿Why did you change this now? ¿Why did you add clsx function in this PR?
It's not a braking thing, the code will be as good as before but let's try to not do PR's with so many tasks because this particular change is not compleate/shouldn't be done now i think.
@@ -412,23 +415,28 @@ export function GameboardContainer({ | |||
</div> | |||
<div className={styles.mazeContainer} ref={gameboardRef} tabIndex={0}> | |||
<div className={styles.toolbar}> | |||
{earnedButNotMintedCheddar > 0 && ( | |||
{earnedButNotMintedCheddar > 100 && ( |
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.
I forgot to check this. Wait till i do plz.
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.
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.
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.
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.
@Mart-Dan-Rossi did you remove the existing data from /db
, because the structure of data changed after we added base, so maybe that might be the reason?
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.
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.
https://drive.google.com/file/d/1Ps9CdDtGCxzKrZTVn42GeyM0a-OF9Rqo/view?usp=sharing
Yeah, that's what i saw. I was thinking that if we can't make the button work properly BECAUSE we don't have the mint keys this functionallity is flat wong. In production users won't have mint keys.
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.
oh that's because the backend server has the keys, so the frontend user don't need it. It will work once we deploy the code to production.
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.
Hmmm.. And if it doesn't? I'm not sure how to handdle this situation. @RodrigoCSolari any suggestion?
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.
I guess, he can deploy it on testnet (the backend, which has the keys) and then we can test it.
15a6205
to
32af05f
Compare
Not able to recreated / removed from this PR