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

[#8529] RatingsBox #1694

Merged
merged 1 commit into from
Nov 28, 2024
Merged

[#8529] RatingsBox #1694

merged 1 commit into from
Nov 28, 2024

Conversation

vellip
Copy link
Collaborator

@vellip vellip commented Nov 19, 2024

adapt ratings buttons to have new icons, more modern code and tests, and a functioning redirect when unauthenticated

Tasks

  • PR name contains story or task reference
  • Documentation (docs and inline)
  • Tests (including n+1 and django_assert_num_queries where applicable)
  • Changelog

Example of how the render prop can be used

https://github.com/liqd/a4-meinberlin/blob/9199aac9a351a130d84f738f304e210a767221e5/meinberlin/react/budgeting/SupportBox.jsx
A custom rating component that allows only positive ratings. All the UI changes can be handled in meinBerlin and the functionality (redirecting when unauthenticated, REST-API handling of ratings, updating state) stays in a4.

@goapunk not sure anymore, do I have to change the hashes in requirements.txt and package.json in this PR?

@vellip vellip requested review from goapunk and hom3mad3 November 19, 2024 16:25
@vellip vellip force-pushed the pv-2024-11-react-ratings branch from 6a0c2e3 to 267af3d Compare November 19, 2024 16:36
@vellip
Copy link
Collaborator Author

vellip commented Nov 19, 2024

Wait, I guess it doesn't make sense to change it in this PR, I meant, do I have to change it in the mb PR? Or only after this PR is merged so it points to the main branch?

@vellip vellip force-pushed the pv-2024-11-react-ratings branch 2 times, most recently from ea2bf02 to 0d97967 Compare November 20, 2024 07:25
@goapunk
Copy link
Contributor

goapunk commented Nov 20, 2024

Wait, I guess it doesn't make sense to change it in this PR, I meant, do I have to change it in the mb PR? Or only after this PR is merged so it points to the main branch?

@vellip we usually update the mb pr with a new commit updating the hash in package.json and requirements/base.xt once the a4 pr is merged

@goapunk
Copy link
Contributor

goapunk commented Nov 27, 2024

@vellip sorry, I haven't looked at the changes yet but just for planning the next releases: is this a breaking change, e.g. for regular mb (not dev) and a+ ? If so could you include a note in the changelog (e.g. prefix the changelog entry with Breaking Change or something). Thanks!

@goapunk
Copy link
Contributor

goapunk commented Nov 27, 2024

@vellip ah, sorry, you mentioned that in the PR description already: All the UI changes can be handled in meinBerlin and the functionality (redirecting when unauthenticated, REST-API handling of ratings, updating state) stays in a4.
nevermind :)

Copy link
Contributor

@goapunk goapunk left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks. Will leave for @hom3mad3 as you tested the corresponding mb pr as well. Just left some small comments/questions

adhocracy4/ratings/static/ratings/RatingBox.jsx Outdated Show resolved Hide resolved
@hom3mad3
Copy link
Contributor

@vellip looks great! i'll let you merge after reading the pending comments :)

@vellip vellip force-pushed the pv-2024-11-react-ratings branch from 0d97967 to 0e1157b Compare November 28, 2024 17:44
@vellip vellip merged commit 91852f5 into main Nov 28, 2024
5 checks passed
@vellip vellip deleted the pv-2024-11-react-ratings branch November 28, 2024 17:58
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.

3 participants