Reporting: Fix the View report link URL on Total payment volume tile, within Payment activity widget
#8726
Conversation
Test the buildOption 1. Jetpack Beta
Option 2. Jurassic Ninja - available for logged-in A12s🚀 Launch a JN site with this branch 🚀 ℹ️ Install this Tampermonkey script to get more options. Build info:
Note: the build is updated when a new commit is pushed to this PR. |
|
Size Change: +116 B (0%) Total Size: 1.25 MB
ℹ️ View Unchanged
|
If I do not have |
Hi @jessy-p , I hope you are seeing all transaction types except for
Yes, it is expected for you to see all other transaction type tags in the search bar, since we do not have a negative search option .. The types include Since this list is long, to prevent warping I added a subtle scroll overflow. If you like to see the loan related transactions filtered out, feel free to switch to my test account via the CLI command on server - |
|
After using the server branch 🟡 Only if the customer clicks on ✅ Filters are working correctly and when changed, the data refreshes accordingly. Everything clear, nothing confusing or odd observed. 🟡 The scroll section still looks bit off to me. Bottom part of the tags seem to be cut off and when you click in the box, the |
I have addressed this here 75a17d2 It looks like this now with the advanced date filters visible.
|
|
On this point
I tried on Chrome, Vivaldi, FF and Safari at my end, and I am unable to replicate the bottom clipping on the search box tags. The In its current state, the landing link shows the Total Payment Volume numbers and its breakdown. But it makes further filtration by the user a bit uncomfortable. The recording here may illustrate this better. Screen.Capture.on.2024-04-30.at.21-55-53.mov@rogermattic , Hi, we need your help here for the view of the search bar within Transactions report results - when
In our case because of the filter needed for TPV transactions ). This disrupts the appearance of the search prompt. It is possible to add further search tags though, but visibility is a bit bad. @jessy-p I was wondering if I should create a separate case for this. Would this be a blocker for the current PR? |
client/transactions/list/style.scss
Outdated
| @@ -123,6 +123,8 @@ $gap-small: 12px; | |||
|
|
|||
| .woocommerce-select-control__control { | |||
| height: 38px; | |||
| overflow: scroll; | |||
| align-items: flex-start; | |||
There was a problem hiding this comment.
Although it is a minor fix, this change isn't behind the feature flag.
Perhaps it would help communicate this change if we had 2 changelog entries:
- Fix for view report behind feature flag.
- Fix for Transaction List styles
Alternatively, relevant to this comment:
@jessy-p I was wondering if I should create a separate case for this. Would this be a blocker for the current PR?
We could move this change to a separate issue/PR. This may help us step back and look at the problem/solution separate to the Payment Activity Card.
There was a problem hiding this comment.
As an alternative UX to overflow: scroll, I would consider letting the search box to increase in height when necessary, example:
(avoiding the !important if possible 😉)
There was a problem hiding this comment.
Although it is a minor fix, this change isn't behind the feature flag.
I had completely missed this! Thanks for sharing!.
I would favor creating a separate issue + PR. It would be tiny but keep things clear.
I did a deeper check and it looks like we need some more work to do for responsive view ( for narrow screens ) . e.g. The Date filter with Between appears botched in mobile screen, and the search prompt disappears too in the narrow screen. I will create a separate issue for this.
Within this PR however, I will include the change @Jinksi suggested to increase the height. - 75a65a4. Since it will help immediately with how the tags appear when there are too many.
There was a problem hiding this comment.
@nagpai Thanks for rising it and @Jinksi thanks for coming up with the fix. I feel like the increase in height is the best we can do now. For the narrower (mobile) screens - I don't know if this is possible – but I'd:
- put the applied filter (tags) in one line (could they be horizontally scrollable?)
- have the
xcross sign for the user to clear the filters visible - have the next (2nd) line empty so that the user can search if they want
In a nutshell, I'd prioritise the user's ability to search over seeing all the applied filters.
Ugh, does it make sense?
There was a problem hiding this comment.
@rogermattic Thanks, Magda! I will add this guidance in the new issue I create for improving the mobile UI 🙏 !
There was a problem hiding this comment.
Thanks @nagpai ! if you have anything for me to look at, just ping me:)
There was a problem hiding this comment.
I have created #8781 to track this change here!
|
Via - 16d12d2 Moved the view report search params generating function outside, to help add similar terms to other tiles const searchTermsForViewReportLink = {
totalPaymentVolume: [
'charge',
'payment',
'payment_failure_refund',
'payment_refund',
'refund',
'refund_failure',
'dispute',
'dispute_reversal',
'card_reader_fee',
],
};
const getSearchParams = ( searchTerms: string[] ) => {
return searchTerms.reduce(
( acc, term, index ) => ( {
...acc,
[ `search[${ index }]` ]: term,
} ),
{}
);
};For refunds, charges and disputes, there will be similar arrays added like and the |
| ], | ||
| }; | ||
|
|
||
| const getSearchParams = ( searchTerms: string[] ) => { |
There was a problem hiding this comment.
💭 This is not an issue per se (feel free to ignore), but I'd consider moving this function and searchTerms declaration outside of the React component scope (alongside getDateRange) since it doesn't require any component variables/state.
E.g. if it is declared outside the component, it explicitly lets us know it is decoupled from the component and can be used elsewhere, moved, unit tested, etc.
There was a problem hiding this comment.
That is a valuable feedback. Note-to-self on deciding where to scope. 🙌 .
There was a problem hiding this comment.
This tests well @nagpai 🙌
I've tested with the account mentioned in the description and the view report link works as expected.
On iOS, the search input height may not be visually ideal, but it works well.
As you suggest, I agree we should followup on the mobile UI issues with the transaction filters as separate issues.
Fixes #8706
Changes proposed in this Pull Request
The PR fixes the
View reportlink on Total Payment volume tile to match the requirement as follows:Total payment volumeshould show all types created over the last 7 days (or selected time filter) except forLoan disbursementandLoan repaymentNotes
Type isandType is notfilters, but they do not allow selecting more than one types in it, in the current design.needs_responseandwarning_needs_responsetypehas been added in the search fields.View reportURL was edited to include the search terms.A more permanent solution to this would be to have
typeIsandtypeIsNotparams accept arrays. But this may require higher effort to execute and to ensure backward compatibility. I found using the way similar to Disputes a better option for now.Testing instructions
add/type-to-search-by-fieldsLoan disbursementandLoan repayment. See if you can switch toacct_1OuYn1QpUShfetUAwhich has these ( and should remain for the next few days. Else create the loan transactions using the steps in the PdjTHR-31T-p2.Payments > Overviewand click theView reportagainstTotal payment volumeScreenshots
On a wider screen
On a narrow screen
Without the CSS change, it would appear like this:
npm run changelogto add a changelog file, choosepatchto leave it empty if the change is not significant. You can add multiple changelog files in one PR by running this command a few times.Post merge