-
Notifications
You must be signed in to change notification settings - Fork 191
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
Feature: Default form tooltip #7590
base: epic/campaigns
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.
@alaca this seems more complex than I was expecting - especially for a single use notice.
It looks like we are having to modify/extend the existing notice system to make this work - updating the store, introducing a dismiss option, and a new <Custom />
notice type.
Does this need to be a registered notice or can we simplify this?
@kjohnson my initial idea was to do exactly as you said, write a simple component that will be shown when needed. But then I got an idea to use the existing system, and having a custom and flexible notice option made sense. The biggest advantage of this approach is that any add-on can register a notification. I think we will have more and more stuff like this and having a system in place would help us in the long run. But, I understand your concern regarding added complexity. I can refactor this and use a simplified approach. |
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.
@alaca I'm getting the following error when I try to compile the assets:
ERROR in ./src/DonationForms/V2/resources/components/DonationFormsListTable.tsx 406:0-87
Module not found: Error: Can't resolve '@givewp/campaigns/admin/components/Notifications' in 'C:\Users\glaub\Local Sites\givewp\app\public\wp-content\plugins\give\src\DonationForms\V2\resources\components'
Did you miss the leading dot in 'resolve.extensions'? Did you mean '[".*",".wasm",".mjs",".js",".jsx",".json",".ts",".tsx"]' instead of '["*",".wasm",".mjs",".js",".jsx",".json",".ts",".tsx"]'?
@glaubersilva run |
@glaubersilva hmm... that's strange. |
@alaca If I register the I also tried to replace this:
With this:
And I was able to compile the assets with this approach as well. But the All Forms keeps breaking, so I'm still not able to see it working. |
@glaubersilva I'm using an alias, this should work. The path is correct and I'm not sure why it doesn't work for you. Do me a favor and remove |
@alaca Which versions of |
@glaubersilva I resolved the issue with the alias. I keep forgetting that we have two configuration files for assets @kjohnson tooltip component logic is now simplified |
@alaca I'm using another machine, and I can see it working now. So, it is probably related to some specific settings problem. About the feature, when I set another form to be the default one, it keeps displaying it in the same position instead of displaying it as the first item of the list table. I probably should have implemented it when I did the "Make as default" row action, but I didn't. Do you mind including it here in this PR? If you are too busy I can take a look at it in a subsequent PR. |
Nope, the alias config was missing from
Already included in this PR. |
Actually, it's not. Let me include that |
@alaca Thanks man! About it:
As I mentioned here, I already had set up the alias in this file previously to run some tests, but the All Forms page got broken after that, today later I'll check in the other machine if it still keeps happening or not. |
I have no idea how to do this on frontend with our |
@alaca At the top of my head, I have no idea. I thought changing the order of the items on the backend would be enough, but as we can reorder items in the frontend by clicking in a few column headers, it will probably require a deeper investigation and a refactor in the List Table component to add some kind of option to sticky a specific item at the top. As a note, I checked this branch in the machine where I was getting the problem before and the compiling is working as expected now without breaking the All Forms page. |
@@ -330,6 +352,9 @@ export default function DonationFormsListTable() { | |||
</a> | |||
</> | |||
)} | |||
{state.showDefaultFormTooltip && ( |
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.
@alaca We need to use the isCampaignDetailsPage
here as well to prevent displaying the tooltip on the All Forms page as you can check on the attached screenshot.
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.
Good catch! This page will not be in use after we merge campaigns, so it's not an issue.
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.
Actually, there is the possibility of creating a feature flag that allows old customers to keep this page visible.
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.
Resolved dc269fe
Description
This PR adds a new tooltip that highlights the default form in the campaigns forms list table.
Affects
Donation forms list table
Visuals
Screen.Recording.2024-10-28.at.09.43.07.mov
Testing Instructions
Pre-review Checklist
@unreleased
tags included in DocBlocks