Reporting: Add a parent wrapper component for Payment Activity #8412
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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: +144 B (0%) Total Size: 1.2 MB
ℹ️ View Unchanged
|
nagpai
changed the title
There was a problem hiding this comment.
Thanks, @nagpai, for kicking it off 🎉 , nice one, though for merging let's wait while we align on the execution strategy.
nagpai
added
the
status: blocked
added 7 commits
March 19, 2024 10:10
|
Marking the PR as |
client/overview/index.js
Outdated
| ) } | ||
| { | ||
| // eslint-disable-next-line camelcase | ||
| has_active_loan && ( |
There was a problem hiding this comment.
Suggested change
| has_active_loan && ( | |
| hasActiveLoan && ( |
nagpai
removed
the
status: blocked
Jinksi
reviewed
Mar 27, 2024
Jinksi
reviewed
Mar 27, 2024
Comment on lines
+15
to
+19
| <Card className="wcpay-payments-activity__card"> | ||
| <CardHeader | ||
| className="wcpay-payments-activity__card__header" | ||
| isBorderless={ true } | ||
| > |
There was a problem hiding this comment.
💭 There may be an opportunity to reduce the amount of custom css applied here.
I attempted the following and see no major difference with the rendered component (yet, I know there may be more to come, but we can let that be implemented as-needed).
Suggested change
| <Card className="wcpay-payments-activity__card"> | |
| <CardHeader | |
| className="wcpay-payments-activity__card__header" | |
| isBorderless={ true } | |
| > | |
| <Card> | |
| <CardHeader> |
There was a problem hiding this comment.
Thanks, @Jinksi. I have added these classes since:
-
I found that the Card by default does not have any padding and the inner individual constituents have their own padding. I thought it is better to add the padding to the whole wrapper as per our design, for uniformity. This will also help have same padding for upcoming components.
-
The CSS on header card removes its own padding and there is an
isBorderlessprop set totrueto remove bottom border on the header.
I hope this makes sense.
There was a problem hiding this comment.
Agree with @Jinksi here – not so much "let's avoid css classes", more that we should avoid overriding / customising styles.
If the components aren't doing what we want (i.e. differ from design in subtle or major ways), we should:
- adapt the design to match the components - i.e. we can implement the intended design but it looks a little different (though likely more consistent with rest of app)
- or
- adapt the components so they have the flexibility we need - i.e. this design proposes changes or additions to components, and we need to implement in component
There was a problem hiding this comment.
Thanks for sharing some very helpful guidance. I removed the custom css, and found the only difference it made was adding some extra 8 px padding. I also now see that has been removed in the new simplified design update, making it consistent with the headings of other widgets.
The latest design also has a bottom border ( default view ) for the card header.
naman03malhotra
approved these changes
Mar 27, 2024
11 tasks
13 tasks
|
I am reopening the branch to address the feedback here |
6 tasks
nagpai
added a commit
that referenced
this pull request
Apr 1, 2024
…ses added in #8412 (#8507) Co-authored-by: Nagesh Pai <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8389
Changes proposed in this Pull Request
Payment Activitywidget, that will be visible on thePayment Overviewpage ( Payments > Overview )Payment Activitywidget behind a feature flag -_wcpay_feature_payment_overview_widget. It will appear only when this flag is set to true. ( meant to hide the widget during development phase )Additional context
Add Payment Activity's empty state and filters [WIP] #8234
Testing instructions
Payments > Overviewwp option update _wcpay_feature_payment_overview_widget 1npm 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