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 13: trait values inline editing #58

Conversation

KitanGarcia
Copy link
Contributor

This is addressing issue #13:

I added the ability to edit trait values inline. Each time the user edits and takes their focus off the input element, the values are saved. I also added error checking and surfaced issues to the user via alert messages.

A couple important things to note:

  • I created the TraitValuesRow component, which consists of 2 input fields, one for the name and one for the rarity.
  • In order for the rarity to work correctly as an input, I needed to change rarity in the TraitValues model to be either a number or a string. This is because it would not allow adding 0s after a decimal point.

I'm happy to go back and tweak some things if need be!

@cosimo-rip
Copy link
Contributor

Thanks so much @KitanGarcia! We'll take a look at this asap.

@skeletoncrewrip
Copy link
Collaborator

hi @KitanGarcia - apologies that im only getting back to this now. It looks great but i ran into an issue that could be improved:

If you use "Add a List of Values" to add many trait values at once, they are initialized to a value of "-1". When i try to make changes to the -1 later with your inline editing, it throws an error that the value must be between 0 and 1, even if I am changing it to something in that range.

I get this as soon as I try to type a character:

Screen Shot 2022-03-11 at 11 38 24 AM

@skeletoncrewrip
Copy link
Collaborator

style-wise, it would also be great if we could introduce a small amount of inner padding on the text fields

@@ -18,7 +18,7 @@ import { Traits } from "./trait";
export default interface TraitValue {
id: string;
name: string;
rarity: number;
rarity: number | string; // string to allow . or trailing 0s in inline editing
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we convert to number before storing in the model?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes! This should actually be taken care of already on line 90 of components/TraitValuesRow/index.tsx
Edit: Now on line 77
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok, can we revert the change here then?

@KitanGarcia
Copy link
Contributor Author

@skeletoncrewrip Thanks for taking a look at this and for the feedback! Apologies for not catching this. I rewrote a lot of the code in a way that handles things much more nicely, in my opinion, to solve this, and also added the ability to update values by pressing enter.

hi @KitanGarcia - apologies that im only getting back to this now. It looks great but i ran into an issue that could be improved:

If you use "Add a List of Values" to add many trait values at once, they are initialized to a value of "-1". When i try to make changes to the -1 later with your inline editing, it throws an error that the value must be between 0 and 1, even if I am changing it to something in that range.

I get this as soon as I try to type a character:

Screen Shot 2022-03-11 at 11 38 24 AM

@KitanGarcia
Copy link
Contributor Author

style-wise, it would also be great if we could introduce a small amount of inner padding on the text fields

I went ahead and added p-2 to both inputs. I agree that this looks better.
image

@skeletoncrewrip
Copy link
Collaborator

@KitanGarcia this is looking great - should have time to test tomorrow. Only thing i would like to see is removing the change to the traitValue model if it's unnecessary. I'd like to know that we can guarantee that value is always a number and not a string. Thanks!

@KitanGarcia
Copy link
Contributor Author

@KitanGarcia this is looking great - should have time to test tomorrow. Only thing i would like to see is removing the change to the traitValue model if it's unnecessary. I'd like to know that we can guarantee that value is always a number and not a string. Thanks!

@skeletoncrewrip I was able to restore the traitValue model to its previous state, so rarity will once again always be a Number. I also added protection against another edge case that resulted in buggy behavior if the user added a list of traits and updated rarity by either pressing enter or focusing to another rarity without changing either of them from the default of -1. Let me know if there's anything else you find in testing!

@skeletoncrewrip skeletoncrewrip merged commit 8435a86 into theskeletoncrew:main Mar 22, 2022
@skeletoncrewrip
Copy link
Collaborator

awesome work, thanks @KitanGarcia! Drop your wallet address (here or DM me: https://twitter.com/cosimo_rip) so that I can send you a contributor NFT :)

@skeletoncrewrip
Copy link
Collaborator

skeletoncrewrip commented Mar 22, 2022

@KitanGarcia also, bonus points if you're interested: immediately updating the Total Rarity and calculated "None" rarity anytime there is an update to the individual rows.

Screen Shot 2022-03-22 at 3 28 19 PM

@KitanGarcia
Copy link
Contributor Author

awesome work, thanks @KitanGarcia! Drop your wallet address (here or DM me: https://twitter.com/cosimo_rip) so that I can send you a contributor NFT :)

Oh, awesome! Thanks a ton, I didn't even expect that! @skeletoncrewrip my wallet address is: EZ7sSyRmS9jQPMnjvwRVtBDhqtE1euDeqenAsiKa5ajK

And sounds good! I'll definitely see if I can implement updating rarity in the coming days!

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.

None yet

3 participants