Add Payment Activity's empty state and filters [WIP] #8234
Conversation
elazzabi
changed the title
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: +2.42 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
There was a problem hiding this comment.
Great work here 👏
Review checklist
| Parameter | Result |
|---|---|
| Approach | ✅ All good |
| Implementation | ✅ All good |
| Automated tests | ✅ All good |
| Naming | ✅ All good |
| Typing | ✅ All good |
| Code comments | 👍 Almost good (left a comment) |
| Changelog | Skipped (until the PR is finalized) |
| Demo (video or JN site) | Skipped (until the PR is finalized) |
| Manual testing | ✅ Works as expected (tried the tpv !==0 hack) |
| } = wcpaySettings; | ||
| const [ before, setBefore ] = useState( moment() ); | ||
| const [ after, setAfter ] = useState( | ||
| moment().subtract( 7, 'days' ).startOf( 'day' ) |
There was a problem hiding this comment.
Is the idea to keep the number 7 always or read it from the filters when they are ready?
There was a problem hiding this comment.
The idea is the default stats will be for the past week, hence using the 7 here. Once the user interacts with the filters, this value will change accordingly.
There was a problem hiding this comment.
Does it make sense to read this value always from the "compared to" filter? We can probably depend on 7 being the default value for that filter.
There was a problem hiding this comment.
I am not against having a default value here, just against the idea of hard-coding. In that way, this component seems to be doing more than its paygrade.
There was a problem hiding this comment.
I agree that we shouldn't hard-code things when it's not needed, but I don't think that depending on the "compared to" filter is the way to go.
I think the two should be separate or at least the other way around, "Compared to:" depending on the "From:" filter. Users will select a date from the "From:" filter first and expect to have the "Compared to:" automatically follow the time range.
However, I believe these will be more apparent when starting the "data connection" phase. So if I'm handing this over to another team, I prefer to keep it separate so they can work on their preferred logic afterward. Otherwise, I might be introducing an extra burden to understand my logic...
What do you think?
There was a problem hiding this comment.
Yep, that makes sense 👍 WDYT about putting a TODO note about it in code and keeping this thread open (unresolved)?
There was a problem hiding this comment.
Agree! Added a TODO and linked to this thread 2903a10
client/overview/index.js
Outdated
| <Welcome /> | ||
| <AccountBalances /> | ||
| </Card> | ||
| <Card> |
There was a problem hiding this comment.
Things have started to get repetitive in the if-else conditions. Time for a refactor?
Was rendering a <button> inside a <button> which isn't a correct HTML DOM
… favor of a cleaner way
|
Not including a changelog here as this will depend on the team taking this forward... |
elazzabi
changed the title
|
Excellent job, @elazzabi ! The code appears to be well done. Something I observed is that tracks events are not yet implemented for both the empty state and filter interactions. It would be beneficial to have them in accordance with the acceptance criteria. |
|
Handover notes pepjwh-Ok-p2#comment-926 |
|
Closing the PR since it was handed over and addressed via #8558 |
Fixes #8198
Changes proposed in this Pull Request
Testing instructions
tpv === 0totpv !== 0npm 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