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

feat: recent unpaid bounties selection #1589

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

aegroto
Copy link
Contributor

@aegroto aegroto commented Nov 13, 2024

Description

Added an option to show only recent unpaid bounties. Fixes #1586

Screenshots

The current bounties selection shows both paid and unpaid bounties:

immagine

The introduced unpaid bounties option filters out paid bounties and show only available ones:

immagine

Additional Context

I have some concerns, mainly due to to my lack of experience in the project:

  1. This is the first selection option that includes a space inside. May it be a source of problems in the future?
  2. The additional selection has been implemented as a separate option in the dropdown menu. Is there a way to add it as an additional option, such as a checkbox, while showing the "bounties" results? That would be interesting but as of now the query is merely done in terms of the route.
  3. The option has been added to the ITEM_TYPES constant. Should it be added to the ITEM_TYPES_USER as well? Where is this other constant used?

Checklist

Are your changes backwards compatible? Please answer below: Yes

On a scale of 1-10 how well and how have you QA'd this change and any features it might affect? Please answer below: 8, I have tested it manually but were unable to add automated tests.

For frontend changes: Tested on mobile? Please answer below: Yes, everything works.

Did you introduce any new environment variables? If so, call them out explicitly here: No

@ekzyis ekzyis self-requested a review November 13, 2024 22:09
@ekzyis
Copy link
Member

ekzyis commented Nov 13, 2024

The additional selection has been implemented as a separate option in the dropdown menu. Is there a way to add it as an additional option, such as a checkbox, while showing the "bounties" results? That would be interesting but as of now the query is merely done in terms of the route.

Yes, this should be possible and I would prefer this a lot more than having another dedicated option:

2024-11-13-231346_475x138_scrot

using "active only" instead of simply "active" might be a bad decision

We don't use dynamic checkboxes anywhere in this context afaik but we have dynamic dropdowns depending on what you selected in top. Maybe the code for them is inspiring.

@ekzyis ekzyis added the feature new product features that weren't there before label Nov 13, 2024
@aegroto
Copy link
Contributor Author

aegroto commented Nov 14, 2024

I have modified the widget to use a checkbox instead of a separate select option:

immagine

The dropdown is still redirected to "bounties", I believe the UX is fluid enough. Let me know what you think of this reworked version :)

@ekzyis
Copy link
Member

ekzyis commented Nov 15, 2024

Thank you, I will take a look tomorrow

@ekzyis
Copy link
Member

ekzyis commented Nov 17, 2024

Sorry, forgot about this yesterday. Will review now

Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

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

Works great, thank you!

2024-11-17.23-43-58.mp4

However, I found some aesthetic issues in the frontend code, see suggestions.

components/recent-header.js Outdated Show resolved Hide resolved
components/recent-header.js Outdated Show resolved Hide resolved
components/recent-header.js Outdated Show resolved Hide resolved
components/recent-header.js Outdated Show resolved Hide resolved
@aegroto
Copy link
Contributor Author

aegroto commented Nov 17, 2024

Hello, I totally agree with your review. I should've used a query param since the beginning, that was the most straightforward solution, sorry for that. I will correct the code and update the PR as soon as I can.

@aegroto
Copy link
Contributor Author

aegroto commented Nov 18, 2024

Thanks for your suggestion, I have applied them to my branch and tested that everything keeps working. I have decided to refactor variablesFunc adding explicit conditionals to avoid the nested ternary operators, but let me know if you prefer to keep with the previous version.

Copy link

gitguardian bot commented Nov 21, 2024

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
Once a secret has been leaked into a git repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.


🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.

ekzyis
ekzyis previously approved these changes Nov 21, 2024
Copy link
Member

@ekzyis ekzyis left a comment

Choose a reason for hiding this comment

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

I have decided to refactor variablesFunc adding explicit conditionals to avoid the nested ternary operators, but let me know if you prefer to keep with the previous version.

Thank you, that was a good decision!

Looks good to me, I also tested again.

ignore the warning from @GitGuardian, the new "secrets" we pushed are only for local nodes running on regtest

@ekzyis ekzyis dismissed their stale review November 21, 2024 02:18

Oh, I think we should use router.replace instead of router.push but I need to check

@aegroto
Copy link
Contributor Author

aegroto commented Nov 21, 2024

@ekzyis Thanks for your feedback. I am curious, why would prefer to use replace instead of push? Anyway, once you take the decision I will update the PR accordingly.

@ekzyis
Copy link
Member

ekzyis commented Nov 21, 2024

@ekzyis Thanks for your feedback. I am curious, why would prefer to use replace instead of push? Anyway, once you take the decision I will update the PR accordingly.

Because clicking the checkbox should not push a new URL to prevent backward navigation, see video:

2024-11-21.15-30-01.mp4

Additionally, as I noticed in the video, I think it makes sense to populate the filter based on the query parameter.

This might be opinionated, but I also think you shouldn't pass a hook value like router via props but use useRouter again in the component.

@aegroto
Copy link
Contributor Author

aegroto commented Nov 21, 2024

I totally agree with your suggestions, thanks for your revision, I have updated the code accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new product features that weren't there before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filter out paid bounties
2 participants