-
Notifications
You must be signed in to change notification settings - Fork 14
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: Enable Recent Activity Table Item Redo Functionality #3587
base: master
Are you sure you want to change the base?
Conversation
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.
Nice, now we're able to redo an action from both the Recent activity
table on Dashboard
and from the table on the Activity
page @rumzledz 🎉
Screen.Recording.2024-11-01.at.09.12.33.mov
Screen.Recording.2024-11-01.at.09.12.57.mov
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.
Really nice 🙌
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.
Nice job implementing this and creating the new hook, all looks great and works nicely!
The only reason I'm rejecting is that not all actions should have the redo option:
I'm not sure off the top of my head which actions this affects, but removing permissions is definitely one, and I think maybe some of the advanced payment types?
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've tested this with Simple Payments and which work as expected, however, as Sam noted, not all actions require it, so this needs to be enabled on a case by case basis
Screencast.from.2024-11-01.18-31-53.mp4
Hey @rdig @iamsamgibbs all right noted. This seems to be a wider bug since the Colony Actions have had this Redo Action for all actions for the past 2 months and no one noticed. I'll see what actions are allowed to have this Redo button, will amend the logic for both the Dashboard and Activity page's actions table and will update the issue's dev hours field 👌 |
ae83c58
to
654f1e5
Compare
Hey @iamsamgibbs @rdig I've pushed the required changes. Catch you both on Monday! 👋 |
4b5149b
to
a3b7d19
Compare
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.
Great job with this!
Nice implementation. I agree with your comments that it would be nice to rework the completed action component so that there is a single source of truth for this. I was actually thinking earlier about how spread out everything to do with actions is, and that we could either benefit with unifying aspects of it (I believe the work Chris is doing will help with this), or at the very least, the next time we add a new action we could document everywhere new code is needed as it is so spread out.
Thanks for adding the new functionality around when to show / not show the redo action, appreciate it was outside of the original scope, but you've got it working very nicely.
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.
Really nice solution with the map, it's much nicer and much more unified this way
Create a new team doesn't have Redo ✔️
Transfer funds works ✔️
Simple payment ✔️
Permissions also work ✔️
Activity feed also works 💯
Nice work with solving this, it's quite a simple hook which does one thing only, super legit 💪
Hey @bassgeta @iamsamgibbs! @rdig has just helped me uncover that the Activity table cannot currently redo Reputation-related actions. So I had to push that which got rid of your reviews 😅 The latest commit also includes a fix for preventing the app from crashing when redoing Reputation-related actions, which is an issue raised in #3593 I thought it made sense to combine these fixes because they intertwine. I will be updating the test steps shortly so you can verify that the app doesn't crash when redoing a Reputation action 👌 |
819c711
to
3238cfc
Compare
3238cfc
to
c13ad29
Compare
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.
Nice turnaround of this. Tested and everything works as expected.
Also, nice job in centralizing the redo actions.
Redo simple payment
Screencast.from.2024-11-06.17-23-45.mp4
No redo on a "Create Team" action
Screencast.from.2024-11-06.17-29-07.mp4
Award reputation redo does not crash the app
Screencast.from.2024-11-06.17-33-51.mp4
c13ad29
to
0735bfe
Compare
@iamsamgibbs @rdig @iamsamgibbs apologies guys, I had to resolve some merge conflicts in |
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.
Reapproved
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.
Reapproving 👍
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 just went to test this and make sure redoing Manage Permissions was still working, but it's suffering from the action form not working issue. Can you rebase it @rumzledz ?
Ah right of course, new fixes were added to |
0735bfe
to
870d8ad
Compare
@iamsamgibbs rebased! 🚀 |
Description
This PR enables the Dashboard page's Activity Table Item Redo Function and aligns it with Activity page's Activity Table.
Testing
Note
I had to fix the following existing
master
bug and do some refactoring as well. It is unrelated to issue #3551 but you know what, it makes sense to include it in this PR:@iamsamgibbs "The only reason I'm rejecting is that not all actions should have the redo option"
@rdig "I've tested this with Simple Payments and which work as expected, however, as Sam noted, not all actions require it, so this needs to be enabled on a case by case basis"
I've added a testing section for it 😄
I also had to handle the Redo flow for Reputation-related actions in the Activity Table items so please test that as well.
I'll find a way to refactor the Completed Actions component as well, so we'll have a single source of truth for knowing which actions are not allowed to have a Redo Action. But it will be in a separate future PR if there's capacity 👌
1. Verify that the Redo action button does what it's intended to do
2. Verify that the Redo action is conditionally rendered on a case-by-case basis
useBuildRedoEnabledActionsMap.ts
, after line 116:Note
The pagination might reset back to page 1 but this is already being fixed by @mmioana in PR: #3566
create-data
script and click its kebab menu3. Verify that you can redo Reputation actions and that the app doesn't crash in the process
Diffs
New stuff ✨
useRedoAction
so it's shared consistently between the Action Tables found with the Dashboard & Activity pages.Resolves #3551
Resolves #3593