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: default for collaborators set to 1 and added dev tag #820

Merged

Conversation

nischal-shetty2
Copy link
Contributor

@nischal-shetty2 nischal-shetty2 commented Aug 21, 2024

Fixes #780

previously the default number of collaborators was randomised between 8-10, which would cause the collaboration transaction to fail. This PR fixes this issue by setting the default value of collaborations to 1 and adds a dev tag as a visual hint.

now if we directly send with 1 collaborator selected by default the transaction succeeds as expected.

Screen.Recording.2024-08-21.at.11.26.52.PM.mov

@theborakompanioni
Copy link
Collaborator

previously the default number of collaborators was randomised between 8-10, which would cause the collaboration transaction to fail. This PR fixes this issue by setting the default value of collaborations to 1 and adds a dev tag as a visual hint.

Hey @nischal-shetty2! Nice! Thanks for the contribution!

One additional change request is to only apply this when the app is in "dev mode" (as stated in the linked issue).
You can use isDevMode from src/constants/debugFeatures.ts! Would that be possible?

@nischal-shetty2
Copy link
Contributor Author

One additional change request is to only apply this when the app is in "dev mode" (as stated in the linked issue). You can use isDevMode from src/constants/debugFeatures.ts! Would that be possible?

Sure i can do that right away!

@nischal-shetty2
Copy link
Contributor Author

Hey @nischal-shetty2! Nice! Thanks for the contribution!

This is my first contribution to this repo, and even though it's a small one, I'm glad to start somewhere.
@theborakompanioni, are there any other changes needed?

@theborakompanioni theborakompanioni self-requested a review August 25, 2024 15:06
@theborakompanioni
Copy link
Collaborator

Hey @nischal-shetty2! Nice! Thanks for the contribution!

This is my first contribution to this repo, and even though it's a small one, I'm glad to start somewhere. @theborakompanioni, are there any other changes needed?

@nischal-shetty2 I do not think the "dev" tag is needed, because this feature is also available in production. Should it be removed? What do you think?

Also, can we use undefined (not '' empty string) for customNumCollaboratorsInput when a user did not touch the input field yet?

@nischal-shetty2
Copy link
Contributor Author

I do not think the "dev" tag is needed, because this feature is also available in production. Should it be removed? What do you think?

@theborakompanioni yes, if this is the case in production as well then its definitely not required! I think it would be best not to show the dev tag

Also, can we use undefined (not '' empty string) for customNumCollaboratorsInput when a user did not touch the input field yet?

on it!

@nischal-shetty2
Copy link
Contributor Author

@theborakompanioni made the changes, could you review it now!

@theborakompanioni
Copy link
Collaborator

@theborakompanioni made the changes, could you review it now!

Made some small changes move the dependency on isDevMode from CollaboratorsSelector to the parent component. Other than that, looks good to me! 🚀

Can you review the changes and verify they work properly on your side as well? Start once with npm run dev:start (changes should apply) and once with npm run start (changes should not apply).

@nischal-shetty2
Copy link
Contributor Author

Can you review the changes and verify they work properly on your side as well? Start once with npm run dev:start (changes should apply) and once with npm run start (changes should not apply).

tested it locally on both dev as well as prod and works perfectly! This can be merged now if everything is good!

@theborakompanioni although i noticed one more thing unrelated to this issue, on the Collaborator fees title
Screenshot 2024-08-28 at 8 17 20 PM
is it supposed to be '...' or does it have to show the value like how Number of collaborators title shows us current selected number

@theborakompanioni
Copy link
Collaborator

Can you review the changes and verify they work properly on your side as well? Start once with npm run dev:start (changes should apply) and once with npm run start (changes should not apply).

tested it locally on both dev as well as prod and works perfectly! This can be merged now if everything is good!

💪

@theborakompanioni although i noticed one more thing unrelated to this issue, on the Collaborator fees title Screenshot 2024-08-28 at 8 17 20 PM is it supposed to be '...' or does it have to show the value like how Number of collaborators title shows us current selected number

This should change to something meaningful as soon as you input an amount!

@theborakompanioni theborakompanioni merged commit b3d846e into joinmarket-webui:devel Aug 28, 2024
@nischal-shetty2 nischal-shetty2 deleted the default-collaborators branch August 28, 2024 15:18
@nischal-shetty2
Copy link
Contributor Author

This should change to something meaningful as soon as you input an amount!

yes yes got it, my bad i didnt input the amount field

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devtools Improvements in tooling
Projects
None yet
Development

Successfully merging this pull request may close these issues.

dev: default for collaborators based on active maker count
2 participants