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

Feature: Default form tooltip #7590

Merged
merged 35 commits into from
Nov 21, 2024
Merged

Conversation

alaca
Copy link
Member

@alaca alaca commented Oct 25, 2024

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

  • Relevant @unreleased tags included in DocBlocks
  • Self Review of code and UX completed

Copy link
Member

@kjohnson kjohnson left a 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?

@alaca
Copy link
Member Author

alaca commented Oct 29, 2024

@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.

Copy link
Contributor

@glaubersilva glaubersilva left a 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"]'?

image

@alaca
Copy link
Member Author

alaca commented Nov 4, 2024

@glaubersilva run npm ci

@glaubersilva
Copy link
Contributor

@alaca It didn't work. I also deleted the node_modules folder, re-installed the packages, and tried to compile again, but I'm still getting the same error.

@kjohnson Is this working for you?

@alaca
Copy link
Member Author

alaca commented Nov 5, 2024

@glaubersilva hmm... that's strange.

@glaubersilva
Copy link
Contributor

glaubersilva commented Nov 8, 2024

@alaca If I register the @givewp/campaigns alias in the webpack.mix.js file I can compile the assets, but it breaks the All Forms list table page - check the attached screenshot for reference.

I also tried to replace this:

import NotificationPlaceholder from '@givewp/campaigns/admin/components/Notifications';

With this:

import NotificationPlaceholder from '../../../../Campaigns/resources/admin/components/Notifications';

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.

image

@alaca
Copy link
Member Author

alaca commented Nov 8, 2024

@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 node_modules (I don't know if this will help) and then run npm ci again.

@alaca
Copy link
Member Author

alaca commented Nov 12, 2024

. @alaca I'm using another machine, and I can see it working now. So, it is probably related to some specific settings problem.

Nope, the alias config was missing from webpack.mix.js.

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.

Already included in this PR.

@alaca
Copy link
Member Author

alaca commented Nov 12, 2024

Already included in this PR.

Actually, it's not. Let me include that

@glaubersilva
Copy link
Contributor

@alaca Thanks man!

About it:

Nope, the alias config was missing from webpack.mix.js.

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.

@alaca
Copy link
Member Author

alaca commented Nov 12, 2024

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 have no idea how to do this on frontend with our ListTable component. The data fetching part is handled by the ListTable component. Looks like mutate does not work. Any ideas?

@glaubersilva
Copy link
Contributor

@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.

…nt-GIVE-1341

# Conflicts:
#	src/Campaigns/resources/admin/components/CampaignDetailsPage/index.tsx
@glaubersilva
Copy link
Contributor

@alaca I noticed that the campaigns list table and the form campaigns list table are using the ascending order by default every time the pages get loaded - check the attached screenshot for reference. We need to make sure to use the descending order by default.

campaigns_ASC
campaign_forms_ASC

@alaca
Copy link
Member Author

alaca commented Nov 20, 2024

@glaubersilva resolved 1b187da

@glaubersilva
Copy link
Contributor

@alaca I pulled your recent changes, but still can see the same problem demonstrated in my previous screenshots.

@alaca
Copy link
Member Author

alaca commented Nov 20, 2024

@glaubersilva resolved 2b8e56b

@glaubersilva
Copy link
Contributor

@alaca I'm still able to reproduce the problem, what fixed it for me here in my local machine was editing the src/Views/Components/ListTable/ListTablePage/index.tsx file and rolling back the sortDirection to desc - Can you please check it? After that, I think this PR will be ready to go but we'll need to create a subsequent PR to implement the feature responsible for sticking the default form at the top of the forms list table.

image

Copy link
Contributor

@glaubersilva glaubersilva left a comment

Choose a reason for hiding this comment

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

@alaca I'm approving it to not delay this PR even more, but please implement this suggestion before merging it just to make sure the items in the list table will be displayed in the proper order. Nice work man, thanks for all the changes implemented!

@alaca
Copy link
Member Author

alaca commented Nov 20, 2024

@glaubersilva resolved 8172df7

@glaubersilva
Copy link
Contributor

@alaca You set the wrong value here: 8172df7

With this value the campaigns list table keeps being displayed in the wrong order.

@alaca alaca merged commit da7aa04 into epic/campaigns Nov 21, 2024
20 checks passed
@alaca alaca deleted the feature/tooltip-component-GIVE-1341 branch November 21, 2024 12:04
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